From f4859fc74c06bf5040e96ef68ddf7102f43cc7db Mon Sep 17 00:00:00 2001 From: Susant Sahani Date: Wed, 15 Feb 2017 10:00:35 +0530 Subject: [PATCH 1/3] networkd: fix drop-in conf directory configs overwriting each other Now we track the sections for example [Address] via line number. Which was fine till we din't had dropins dir. If we have multiple sections which have the ideantical line number in diffrent files we are overwriting these since line number is the key. This patch fixes this by taking filename and line number as key. This fixes [Address] and [Route] section overwriting. fixes: #5141 --- src/network/networkd-address.c | 38 +++++++++++++++----------- src/network/networkd-address.h | 5 ++-- src/network/networkd-network.c | 49 +++++++++++++++++++++++++++++++--- src/network/networkd-network.h | 11 ++++++++ src/network/networkd-route.c | 40 ++++++++++++++++----------- src/network/networkd-route.h | 5 ++-- 6 files changed, 110 insertions(+), 38 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index ffd2e18a45..9e41244eb7 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -53,15 +53,20 @@ int address_new(Address **ret) { return 0; } -int address_new_static(Network *network, unsigned section, Address **ret) { +int address_new_static(Network *network, const char *filename, unsigned section_line, Address **ret) { + _cleanup_network_config_section_free_ NetworkConfigSection *n = NULL; _cleanup_address_free_ Address *address = NULL; int r; assert(network); assert(ret); - if (section) { - address = hashmap_get(network->addresses_by_section, UINT_TO_PTR(section)); + if (section_line > 0) { + r = network_config_section_new(filename, section_line, &n); + if (r < 0) + return r; + + address = hashmap_get(network->addresses_by_section, n); if (address) { *ret = address; address = NULL; @@ -77,9 +82,9 @@ int address_new_static(Network *network, unsigned section, Address **ret) { if (r < 0) return r; - if (section) { - address->section = section; - hashmap_put(network->addresses_by_section, UINT_TO_PTR(address->section), address); + if (section_line > 0) { + address->section = n; + hashmap_put(network->addresses_by_section, n, address); } address->network = network; @@ -88,6 +93,7 @@ int address_new_static(Network *network, unsigned section, Address **ret) { *ret = address; address = NULL; + n = NULL; return 0; } @@ -101,8 +107,10 @@ void address_free(Address *address) { assert(address->network->n_static_addresses > 0); address->network->n_static_addresses--; - if (address->section) - hashmap_remove(address->network->addresses_by_section, UINT_TO_PTR(address->section)); + if (address->section) { + hashmap_remove(address->network->addresses_by_section, address->section); + network_config_section_free(address->section); + } } if (address->link) { @@ -676,7 +684,7 @@ int config_parse_broadcast( assert(rvalue); assert(data); - r = address_new_static(network, section_line, &n); + r = address_new_static(network, filename, section_line, &n); if (r < 0) return r; @@ -723,10 +731,10 @@ int config_parse_address(const char *unit, if (streq(section, "Network")) { /* we are not in an Address section, so treat * this as the special '0' section */ - section_line = 0; - } + r = address_new_static(network, NULL, 0, &n); + } else + r = address_new_static(network, filename, section_line, &n); - r = address_new_static(network, section_line, &n); if (r < 0) return r; @@ -805,7 +813,7 @@ int config_parse_label( assert(rvalue); assert(data); - r = address_new_static(network, section_line, &n); + r = address_new_static(network, filename, section_line, &n); if (r < 0) return r; @@ -844,7 +852,7 @@ int config_parse_lifetime(const char *unit, assert(rvalue); assert(data); - r = address_new_static(network, section_line, &n); + r = address_new_static(network, filename, section_line, &n); if (r < 0) return r; @@ -891,7 +899,7 @@ int config_parse_address_flags(const char *unit, assert(rvalue); assert(data); - r = address_new_static(network, section_line, &n); + r = address_new_static(network, filename, section_line, &n); if (r < 0) return r; diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index bc3b4fc7f3..71a07ea7a3 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -33,10 +33,11 @@ typedef struct Address Address; typedef struct Network Network; typedef struct Link Link; +typedef struct NetworkConfigSection NetworkConfigSection; struct Address { Network *network; - unsigned section; + NetworkConfigSection *section; Link *link; @@ -62,7 +63,7 @@ struct Address { LIST_FIELDS(Address, addresses); }; -int address_new_static(Network *network, unsigned section, Address **ret); +int address_new_static(Network *network, const char *filename, unsigned section, Address **ret); int address_new(Address **ret); void address_free(Address *address); int address_add_foreign(Link *link, int family, const union in_addr_union *in_addr, unsigned char prefixlen, Address **ret); diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 92062ca00c..ab372568de 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -36,6 +36,49 @@ #include "string-util.h" #include "util.h" +static void network_config_hash_func(const void *p, struct siphash *state) { + const NetworkConfigSection *c = p; + + siphash24_compress(c->filename, strlen(c->filename), state); + siphash24_compress(&c->line, sizeof(c->line), state); +} + +static int network_config_compare_func(const void *a, const void *b) { + const NetworkConfigSection *x = a, *y = b; + int r; + + r = strcmp(x->filename, y->filename); + if (r != 0) + return r; + + return y->line - x->line; +} + +const struct hash_ops network_config_hash_ops = { + .hash = network_config_hash_func, + .compare = network_config_compare_func, +}; + +int network_config_section_new(const char *filename, unsigned line, NetworkConfigSection **s) { + NetworkConfigSection *cs; + + cs = malloc0(offsetof(NetworkConfigSection, filename) + strlen(filename) + 1); + if (!cs) + return -ENOMEM; + + strcpy(cs->filename, filename); + cs->line = line; + + *s = cs; + cs = NULL; + + return 0; +} + +void network_config_section_free(NetworkConfigSection *cs) { + free(cs); +} + static int network_load_one(Manager *manager, const char *filename) { _cleanup_network_free_ Network *network = NULL; _cleanup_fclose_ FILE *file = NULL; @@ -76,11 +119,11 @@ static int network_load_one(Manager *manager, const char *filename) { if (!network->stacked_netdevs) return log_oom(); - network->addresses_by_section = hashmap_new(NULL); + network->addresses_by_section = hashmap_new(&network_config_hash_ops); if (!network->addresses_by_section) return log_oom(); - network->routes_by_section = hashmap_new(NULL); + network->routes_by_section = hashmap_new(&network_config_hash_ops); if (!network->routes_by_section) return log_oom(); @@ -385,7 +428,7 @@ int network_apply(Network *network, Link *link) { if (network->ipv4ll_route) { Route *route; - r = route_new_static(network, 0, &route); + r = route_new_static(network, "Network", 0, &route); if (r < 0) return r; diff --git a/src/network/networkd-network.h b/src/network/networkd-network.h index f06828a899..4ce066a764 100644 --- a/src/network/networkd-network.h +++ b/src/network/networkd-network.h @@ -82,6 +82,17 @@ typedef struct DUID { uint8_t raw_data[MAX_DUID_LEN]; } DUID; +typedef struct NetworkConfigSection { + unsigned line; + char filename[]; +} NetworkConfigSection; + +int network_config_section_new(const char *filename, unsigned line, NetworkConfigSection **s); +void network_config_section_free(NetworkConfigSection *network); + +DEFINE_TRIVIAL_CLEANUP_FUNC(NetworkConfigSection*, network_config_section_free); +#define _cleanup_network_config_section_free_ _cleanup_(network_config_section_freep) + typedef struct Manager Manager; struct Network { diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index bde26a42d4..81a065dd27 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -77,15 +77,20 @@ int route_new(Route **ret) { return 0; } -int route_new_static(Network *network, unsigned section, Route **ret) { +int route_new_static(Network *network, const char *filename, unsigned section_line, Route **ret) { + _cleanup_network_config_section_free_ NetworkConfigSection *n = NULL; _cleanup_route_free_ Route *route = NULL; int r; assert(network); assert(ret); - if (section) { - route = hashmap_get(network->routes_by_section, UINT_TO_PTR(section)); + if (section_line > 0) { + r = network_config_section_new(filename, section_line, &n); + if (r < 0) + return r; + + route = hashmap_get(network->routes_by_section, n); if (route) { *ret = route; route = NULL; @@ -103,10 +108,10 @@ int route_new_static(Network *network, unsigned section, Route **ret) { route->protocol = RTPROT_STATIC; - if (section) { - route->section = section; + if (section_line > 0) { + route->section = n; - r = hashmap_put(network->routes_by_section, UINT_TO_PTR(route->section), route); + r = hashmap_put(network->routes_by_section, n, route); if (r < 0) return r; } @@ -117,6 +122,7 @@ int route_new_static(Network *network, unsigned section, Route **ret) { *ret = route; route = NULL; + n = NULL; return 0; } @@ -131,8 +137,10 @@ void route_free(Route *route) { assert(route->network->n_static_routes > 0); route->network->n_static_routes--; - if (route->section) - hashmap_remove(route->network->routes_by_section, UINT_TO_PTR(route->section)); + if (route->section) { + hashmap_remove(route->network->routes_by_section, route->section); + network_config_section_free(route->section); + } } if (route->link) { @@ -673,10 +681,10 @@ int config_parse_gateway(const char *unit, if (streq(section, "Network")) { /* we are not in an Route section, so treat * this as the special '0' section */ - section_line = 0; - } + r = route_new_static(network, NULL, 0, &n); + } else + r = route_new_static(network, filename, section_line, &n); - r = route_new_static(network, section_line, &n); if (r < 0) return r; @@ -715,7 +723,7 @@ int config_parse_preferred_src(const char *unit, assert(rvalue); assert(data); - r = route_new_static(network, section_line, &n); + r = route_new_static(network, filename, section_line, &n); if (r < 0) return r; @@ -757,7 +765,7 @@ int config_parse_destination(const char *unit, assert(rvalue); assert(data); - r = route_new_static(network, section_line, &n); + r = route_new_static(network, filename, section_line, &n); if (r < 0) return r; @@ -835,7 +843,7 @@ int config_parse_route_priority(const char *unit, assert(rvalue); assert(data); - r = route_new_static(network, section_line, &n); + r = route_new_static(network, filename, section_line, &n); if (r < 0) return r; @@ -872,7 +880,7 @@ int config_parse_route_scope(const char *unit, assert(rvalue); assert(data); - r = route_new_static(network, section_line, &n); + r = route_new_static(network, filename, section_line, &n); if (r < 0) return r; @@ -913,7 +921,7 @@ int config_parse_route_table(const char *unit, assert(rvalue); assert(data); - r = route_new_static(network, section_line, &n); + r = route_new_static(network, filename, section_line, &n); if (r < 0) return r; diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index 02f0b27675..4ebfa0f0bd 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -20,12 +20,13 @@ ***/ typedef struct Route Route; +typedef struct NetworkConfigSection NetworkConfigSection; #include "networkd-network.h" struct Route { Network *network; - unsigned section; + NetworkConfigSection *section; Link *link; @@ -52,7 +53,7 @@ struct Route { LIST_FIELDS(Route, routes); }; -int route_new_static(Network *network, unsigned section, Route **ret); +int route_new_static(Network *network, const char *filename, unsigned section_line, Route **ret); int route_new(Route **ret); void route_free(Route *route); int route_configure(Route *route, Link *link, sd_netlink_message_handler_t callback); From fd45e522dd8b6cbfdff32eee1f272a97080387ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 17 Feb 2017 09:26:10 -0500 Subject: [PATCH 2/3] networkd: immediately transfer ownership of route->section The code was not incorrect previously, but I think it's easier to follow the ownership (and the code is more likely to remain correct when updated later on), if freeing of NetworkConfigSection* is immediately made the responsibility of route_free(), so instead of relying on route_free() not freeing ->section if adding to the network hashmap failed, make this freeing unconditional. --- src/network/networkd-route.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 81a065dd27..ca606c3d25 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -110,6 +110,7 @@ int route_new_static(Network *network, const char *filename, unsigned section_li if (section_line > 0) { route->section = n; + n = NULL; r = hashmap_put(network->routes_by_section, n, route); if (r < 0) @@ -122,7 +123,6 @@ int route_new_static(Network *network, const char *filename, unsigned section_li *ret = route; route = NULL; - n = NULL; return 0; } @@ -137,12 +137,12 @@ void route_free(Route *route) { assert(route->network->n_static_routes > 0); route->network->n_static_routes--; - if (route->section) { + if (route->section) hashmap_remove(route->network->routes_by_section, route->section); - network_config_section_free(route->section); - } } + network_config_section_free(route->section); + if (route->link) { set_remove(route->link->routes, route); set_remove(route->link->routes_foreign, route); From 48317c39e26a203dc62d1f707263c75a79f89259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 17 Feb 2017 09:34:25 -0500 Subject: [PATCH 3/3] network: change condition in if testing section presence section_line and filename should be set together or not at all. Change the if to test filename, since it's the first of the pair and it seems more natural to test that. --- src/network/networkd-address.c | 5 +++-- src/network/networkd-route.c | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 9e41244eb7..9d0311d76f 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -60,8 +60,9 @@ int address_new_static(Network *network, const char *filename, unsigned section_ assert(network); assert(ret); + assert(!!filename == (section_line > 0)); - if (section_line > 0) { + if (filename) { r = network_config_section_new(filename, section_line, &n); if (r < 0) return r; @@ -82,7 +83,7 @@ int address_new_static(Network *network, const char *filename, unsigned section_ if (r < 0) return r; - if (section_line > 0) { + if (filename) { address->section = n; hashmap_put(network->addresses_by_section, n, address); } diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index ca606c3d25..a8ba293ae8 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -84,8 +84,9 @@ int route_new_static(Network *network, const char *filename, unsigned section_li assert(network); assert(ret); + assert(!!filename == (section_line > 0)); - if (section_line > 0) { + if (filename) { r = network_config_section_new(filename, section_line, &n); if (r < 0) return r; @@ -108,7 +109,7 @@ int route_new_static(Network *network, const char *filename, unsigned section_li route->protocol = RTPROT_STATIC; - if (section_line > 0) { + if (filename) { route->section = n; n = NULL;