Merge pull request systemd#30550 from yuwata/network-nexthop-cleanups-3

network: several cleanups for nexthop (part3)

src/network/networkd-manager.c

@@ -639,6 +639,7 @@ Manager* manager_free(Manager *m) {
         m->routes = set_free(m->routes);
 
         m->nexthops_by_id = hashmap_free(m->nexthops_by_id);
+        m->nexthop_ids = set_free(m->nexthop_ids);
 
         sd_event_source_unref(m->speed_meter_event_source);
         sd_event_unref(m->event);
@@ -699,7 +700,15 @@ int manager_load_config(Manager *m) {
         if (r < 0)
                 return r;
 
-        return manager_build_dhcp_pd_subnet_ids(m);
+        r = manager_build_dhcp_pd_subnet_ids(m);
+        if (r < 0)
+                return r;
+
+        r = manager_build_nexthop_ids(m);
+        if (r < 0)
+                return r;
+
+        return 0;
 }
 
 int manager_enumerate_internal(

src/network/networkd-manager.h

@@ -75,6 +75,7 @@ struct Manager {
 
         /* Manage nexthops by id. */
         Hashmap *nexthops_by_id;
+        Set *nexthop_ids; /* requested IDs in .network files */
 
         /* Manager stores routes without RTA_OIF attribute. */
         unsigned route_remove_messages;

src/network/networkd-network.c

@@ -306,7 +306,9 @@ int network_verify(Network *network) {
         if (r < 0)
                 return r; /* network_drop_invalid_addresses() logs internally. */
         network_drop_invalid_routes(network);
-        network_drop_invalid_nexthops(network);
+        r = network_drop_invalid_nexthops(network);
+        if (r < 0)
+                return r;
         network_drop_invalid_bridge_fdb_entries(network);
         network_drop_invalid_bridge_mdb_entries(network);
         r = network_drop_invalid_neighbors(network);
@@ -634,7 +636,15 @@ int network_reload(Manager *manager) {
         ordered_hashmap_free_with_destructor(manager->networks, network_unref);
         manager->networks = new_networks;
 
-        return manager_build_dhcp_pd_subnet_ids(manager);
+        r = manager_build_dhcp_pd_subnet_ids(manager);
+        if (r < 0)
+                return r;
+
+        r = manager_build_nexthop_ids(manager);
+        if (r < 0)
+                return r;
+
+        return 0;
 
 failure:
         ordered_hashmap_free_with_destructor(new_networks, network_unref);
@@ -773,7 +783,7 @@ static Network *network_free(Network *network) {
         set_free_free(network->ipv6_proxy_ndp_addresses);
         ordered_hashmap_free_with_destructor(network->addresses_by_section, address_free);
         hashmap_free_with_destructor(network->routes_by_section, route_free);
-        hashmap_free_with_destructor(network->nexthops_by_section, nexthop_free);
+        ordered_hashmap_free_with_destructor(network->nexthops_by_section, nexthop_free);
         hashmap_free_with_destructor(network->bridge_fdb_entries_by_section, bridge_fdb_free);
         hashmap_free_with_destructor(network->bridge_mdb_entries_by_section, bridge_mdb_free);
         ordered_hashmap_free_with_destructor(network->neighbors_by_section, neighbor_free);

src/network/networkd-network.h

@@ -371,7 +371,7 @@ struct Network {
 
         OrderedHashmap *addresses_by_section;
         Hashmap *routes_by_section;
-        Hashmap *nexthops_by_section;
+        OrderedHashmap *nexthops_by_section;
         Hashmap *bridge_fdb_entries_by_section;
         Hashmap *bridge_mdb_entries_by_section;
         OrderedHashmap *neighbors_by_section;

src/network/networkd-nexthop.c

@@ -24,7 +24,7 @@ NextHop *nexthop_free(NextHop *nexthop) {
 
         if (nexthop->network) {
                 assert(nexthop->section);
-                hashmap_remove(nexthop->network->nexthops_by_section, nexthop->section);
+                ordered_hashmap_remove(nexthop->network->nexthops_by_section, nexthop->section);
         }
 
         config_section_free(nexthop->section);
@@ -80,7 +80,7 @@ static int nexthop_new_static(Network *network, const char *filename, unsigned s
         if (r < 0)
                 return r;
 
-        nexthop = hashmap_get(network->nexthops_by_section, n);
+        nexthop = ordered_hashmap_get(network->nexthops_by_section, n);
         if (nexthop) {
                 *ret = TAKE_PTR(nexthop);
                 return 0;
@@ -95,7 +95,7 @@ static int nexthop_new_static(Network *network, const char *filename, unsigned s
         nexthop->section = TAKE_PTR(n);
         nexthop->source = NETWORK_CONFIG_SOURCE_STATIC;
 
-        r = hashmap_ensure_put(&network->nexthops_by_section, &config_section_hash_ops, nexthop->section, nexthop);
+        r = ordered_hashmap_ensure_put(&network->nexthops_by_section, &config_section_hash_ops, nexthop->section, nexthop);
         if (r < 0)
                 return r;
 
@@ -239,6 +239,10 @@ static int nexthop_get(Link *link, const NextHop *in, NextHop **ret) {
         if (in->id > 0)
                 return nexthop_get_by_id(link->manager, in->id, ret);
 
+        /* If ManageForeignNextHops=no, nexthop with id == 0 should be already filtered by
+         * nexthop_section_verify(). */
+        assert(link->manager->manage_foreign_nexthops);
+
         ifindex = nexthop_bound_to_link(in) ? link->ifindex : 0;
 
         HASHMAP_FOREACH(nexthop, link->manager->nexthops_by_id) {
@@ -247,6 +251,11 @@ static int nexthop_get(Link *link, const NextHop *in, NextHop **ret) {
                 if (nexthop_compare_full(nexthop, in) != 0)
                         continue;
 
+                /* Even if the configuration matches, it may be configured with another [NextHop] section
+                 * that has an explicit ID. If so, the assigned nexthop is not the one we are looking for. */
+                if (set_contains(link->manager->nexthop_ids, UINT32_TO_PTR(nexthop->id)))
+                        continue;
+
                 if (ret)
                         *ret = nexthop;
                 return 0;
@@ -287,6 +296,10 @@ static int nexthop_get_request(Link *link, const NextHop *in, Request **ret) {
         if (in->id > 0)
                 return nexthop_get_request_by_id(link->manager, in->id, ret);
 
+        /* If ManageForeignNextHops=no, nexthop with id == 0 should be already filtered by
+         * nexthop_section_verify(). */
+        assert(link->manager->manage_foreign_nexthops);
+
         ifindex = nexthop_bound_to_link(in) ? link->ifindex : 0;
 
         ORDERED_SET_FOREACH(req, link->manager->request_queue) {
@@ -299,6 +312,11 @@ static int nexthop_get_request(Link *link, const NextHop *in, Request **ret) {
                 if (nexthop_compare_full(nexthop, in) != 0)
                         continue;
 
+                /* Even if the configuration matches, it may be requested by another [NextHop] section
+                 * that has an explicit ID. If so, the request is not the one we are looking for. */
+                if (set_contains(link->manager->nexthop_ids, UINT32_TO_PTR(nexthop->id)))
+                        continue;
+
                 if (ret)
                         *ret = req;
                 return 0;
@@ -336,10 +354,6 @@ static int nexthop_add_new(Manager *manager, uint32_t id, NextHop **ret) {
 }
 
 static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) {
-        _cleanup_set_free_ Set *ids = NULL;
-        Network *network;
-        int r;
-
         assert(manager);
         assert(nexthop);
 
@@ -352,25 +366,12 @@ static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) {
 
         /* Find the lowest unused ID. */
 
-        ORDERED_HASHMAP_FOREACH(network, manager->networks) {
-                NextHop *tmp;
-
-                HASHMAP_FOREACH(tmp, network->nexthops_by_section) {
-                        if (tmp->id == 0)
-                                continue;
-
-                        r = set_ensure_put(&ids, NULL, UINT32_TO_PTR(tmp->id));
-                        if (r < 0)
-                                return r;
-                }
-        }
-
         for (uint32_t id = 1; id < UINT32_MAX; id++) {
                 if (nexthop_get_by_id(manager, id, NULL) >= 0)
                         continue;
                 if (nexthop_get_request_by_id(manager, id, NULL) >= 0)
                         continue;
-                if (set_contains(ids, UINT32_TO_PTR(id)))
+                if (set_contains(manager->nexthop_ids, UINT32_TO_PTR(id)))
                         continue;
 
                 nexthop->id = id;
@@ -430,16 +431,14 @@ static int nexthop_remove(NextHop *nexthop) {
         Request *req;
         int r;
 
-        manager = ASSERT_PTR(ASSERT_PTR(nexthop)->manager);
+        assert(nexthop);
+        assert(nexthop->id > 0);
+
+        manager = ASSERT_PTR(nexthop->manager);
 
         /* link may be NULL. */
         (void) link_get_by_index(manager, nexthop->ifindex, &link);
 
-        if (nexthop->id == 0) {
-                log_link_debug(link, "Cannot remove nexthop without valid ID, ignoring.");
-                return 0;
-        }
-
         log_nexthop_debug(nexthop, "Removing", manager);
 
         r = sd_rtnl_message_new_nexthop(manager->rtnl, &m, RTM_DELNEXTHOP, AF_UNSPEC, RTPROT_UNSPEC);
@@ -555,6 +554,7 @@ static bool nexthop_is_ready_to_configure(Link *link, const NextHop *nexthop) {
 
         assert(link);
         assert(nexthop);
+        assert(nexthop->id > 0);
 
         if (!link_is_ready_to_configure(link, false))
                 return false;
@@ -582,17 +582,6 @@ static bool nexthop_is_ready_to_configure(Link *link, const NextHop *nexthop) {
                         return false;
         }
 
-        if (nexthop->id == 0) {
-                Request *req;
-
-                ORDERED_SET_FOREACH(req, link->manager->request_queue) {
-                        if (req->type != REQUEST_TYPE_NEXTHOP)
-                                continue;
-                        if (((NextHop*) req->userdata)->id != 0)
-                                return false; /* first configure nexthop with id. */
-                }
-        }
-
         return gateway_is_ready(link, FLAGS_SET(nexthop->flags, RTNH_F_ONLINK), nexthop->family, &nexthop->gw);
 }
 
@@ -682,7 +671,7 @@ int link_request_static_nexthops(Link *link, bool only_ipv4) {
 
         link->static_nexthops_configured = false;
 
-        HASHMAP_FOREACH(nh, link->network->nexthops_by_section) {
+        ORDERED_HASHMAP_FOREACH(nh, link->network->nexthops_by_section) {
                 if (only_ipv4 && nh->family != AF_INET)
                         continue;
 
@@ -763,7 +752,7 @@ static void link_mark_nexthops(Link *link, bool foreign) {
                 if (!IN_SET(other->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED))
                         continue;
 
-                HASHMAP_FOREACH(nexthop, other->network->nexthops_by_section) {
+                ORDERED_HASHMAP_FOREACH(nexthop, other->network->nexthops_by_section) {
                         NextHop *existing;
 
                         if (nexthop_get(other, nexthop, &existing) < 0)
@@ -1022,20 +1011,34 @@ static int nexthop_section_verify(NextHop *nh) {
                                                  "Ignoring [NextHop] section from line %u.",
                                                  nh->section->filename, nh->section->line);
 
-                if (nh->blackhole && in_addr_is_set(nh->family, &nh->gw))
+                if (nh->blackhole)
                         return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
                                                  "%s: nexthop group cannot be a blackhole. "
                                                  "Ignoring [NextHop] section from line %u.",
                                                  nh->section->filename, nh->section->line);
+
+                if (nh->onlink > 0)
+                        return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                                 "%s: nexthop group cannot have on-link flag. "
+                                                 "Ignoring [NextHop] section from line %u.",
+                                                 nh->section->filename, nh->section->line);
         } else if (nh->family == AF_UNSPEC)
                 /* When neither Family=, Gateway=, nor Group= is specified, assume IPv4. */
                 nh->family = AF_INET;
 
-        if (nh->blackhole && in_addr_is_set(nh->family, &nh->gw))
-                return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
-                                         "%s: blackhole nexthop cannot have gateway address. "
-                                         "Ignoring [NextHop] section from line %u.",
-                                         nh->section->filename, nh->section->line);
+        if (nh->blackhole) {
+                if (in_addr_is_set(nh->family, &nh->gw))
+                        return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                                 "%s: blackhole nexthop cannot have gateway address. "
+                                                 "Ignoring [NextHop] section from line %u.",
+                                                 nh->section->filename, nh->section->line);
+
+                if (nh->onlink > 0)
+                        return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                                 "%s: blackhole nexthop cannot have on-link flag. "
+                                                 "Ignoring [NextHop] section from line %u.",
+                                                 nh->section->filename, nh->section->line);
+        }
 
         if (nh->onlink < 0 && in_addr_is_set(nh->family, &nh->gw) &&
             ordered_hashmap_isempty(nh->network->addresses_by_section)) {
@@ -1053,14 +1056,68 @@ static int nexthop_section_verify(NextHop *nh) {
         return 0;
 }
 
-void network_drop_invalid_nexthops(Network *network) {
+int network_drop_invalid_nexthops(Network *network) {
+        _cleanup_hashmap_free_ Hashmap *nexthops = NULL;
         NextHop *nh;
+        int r;
 
         assert(network);
 
-        HASHMAP_FOREACH(nh, network->nexthops_by_section)
-                if (nexthop_section_verify(nh) < 0)
+        ORDERED_HASHMAP_FOREACH(nh, network->nexthops_by_section) {
+                if (nexthop_section_verify(nh) < 0) {
                         nexthop_free(nh);
+                        continue;
+                }
+
+                if (nh->id == 0)
+                        continue;
+
+                /* Always use the setting specified later. So, remove the previously assigned setting. */
+                NextHop *dup = hashmap_remove(nexthops, UINT32_TO_PTR(nh->id));
+                if (dup) {
+                        log_warning("%s: Duplicated nexthop settings for ID %"PRIu32" is specified at line %u and %u, "
+                                    "dropping the nexthop setting specified at line %u.",
+                                    dup->section->filename,
+                                    nh->id, nh->section->line,
+                                    dup->section->line, dup->section->line);
+                        /* nexthop_free() will drop the nexthop from nexthops_by_section. */
+                        nexthop_free(dup);
+                }
+
+                r = hashmap_ensure_put(&nexthops, NULL, UINT32_TO_PTR(nh->id), nh);
+                if (r < 0)
+                        return log_oom();
+                assert(r > 0);
+        }
+
+        return 0;
+}
+
+int manager_build_nexthop_ids(Manager *manager) {
+        Network *network;
+        int r;
+
+        assert(manager);
+
+        if (!manager->manage_foreign_nexthops)
+                return 0;
+
+        manager->nexthop_ids = set_free(manager->nexthop_ids);
+
+        ORDERED_HASHMAP_FOREACH(network, manager->networks) {
+                NextHop *nh;
+
+                ORDERED_HASHMAP_FOREACH(nh, network->nexthops_by_section) {
+                        if (nh->id == 0)
+                                continue;
+
+                        r = set_ensure_put(&manager->nexthop_ids, NULL, UINT32_TO_PTR(nh->id));
+                        if (r < 0)
+                                return r;
+                }
+        }
+
+        return 0;
 }
 
 int config_parse_nexthop_id(

src/network/networkd-nexthop.h

@@ -37,7 +37,7 @@ typedef struct NextHop {
 
 NextHop *nexthop_free(NextHop *nexthop);
 
-void network_drop_invalid_nexthops(Network *network);
+int network_drop_invalid_nexthops(Network *network);
 
 int link_drop_nexthops(Link *link, bool foreign);
 static inline int link_drop_foreign_nexthops(Link *link) {
@@ -52,6 +52,7 @@ int link_request_static_nexthops(Link *link, bool only_ipv4);
 
 int nexthop_get_by_id(Manager *manager, uint32_t id, NextHop **ret);
 int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, Manager *m);
+int manager_build_nexthop_ids(Manager *manager);
 
 DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(NextHop, nexthop);