From 0beb9542e90ab1c5d1507a1046a326fbcf73861c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Apr 2019 20:56:01 +0900 Subject: [PATCH 1/3] network: logs link state change --- src/network/networkd-link.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 4846b13a8b..49363299e5 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -721,6 +721,10 @@ static void link_set_state(Link *link, LinkState state) { if (link->state == state) return; + log_link_debug(link, "State changed: %s -> %s", + link_state_to_string(link->state), + link_state_to_string(state)); + link->state = state; link_send_changed(link, "AdministrativeState", NULL); From bd08ce56156751d58584a44e766ef61340cdae2d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Apr 2019 17:34:00 +0900 Subject: [PATCH 2/3] network: prevent interfaces to be initialized multiple times When a uevent is received during the relevant interface is in LINK_STATE_PENDING, then the interface may be initialized twice. To prevent that, this introduces LINK_STATE_INITIALIZED. --- src/network/networkd-link.c | 15 ++++++++++----- src/network/networkd-link.h | 13 +++++++------ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 49363299e5..3bc7f69cb3 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1455,7 +1455,7 @@ static int set_mtu_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) log_link_debug(link, "Setting MTU done."); - if (link->state == LINK_STATE_PENDING) + if (link->state == LINK_STATE_INITIALIZED) (void) link_configure_after_setting_mtu(link); return 1; @@ -2644,7 +2644,7 @@ static int link_enter_join_netdev(Link *link) { assert(link); assert(link->network); - assert(link->state == LINK_STATE_PENDING); + assert(link->state == LINK_STATE_INITIALIZED); link_set_state(link, LINK_STATE_CONFIGURING); @@ -3057,7 +3057,7 @@ static int link_configure(Link *link) { assert(link); assert(link->network); - assert(link->state == LINK_STATE_PENDING); + assert(link->state == LINK_STATE_INITIALIZED); if (STRPTR_IN_SET(link->kind, "can", "vcan")) return link_configure_can(link); @@ -3206,7 +3206,7 @@ static int link_configure_after_setting_mtu(Link *link) { assert(link); assert(link->network); - assert(link->state == LINK_STATE_PENDING); + assert(link->state == LINK_STATE_INITIALIZED); if (link->setting_mtu) return 0; @@ -3357,10 +3357,13 @@ static int link_initialized_and_synced(Link *link) { assert(link->ifname); assert(link->manager); - if (link->state != LINK_STATE_PENDING) + /* We may get called either from the asynchronous netlink callback, + * or directly for link_add() if running in a container. See link_add(). */ + if (!IN_SET(link->state, LINK_STATE_PENDING, LINK_STATE_INITIALIZED)) return 1; log_link_debug(link, "Link state is up-to-date"); + link_set_state(link, LINK_STATE_INITIALIZED); r = link_new_bound_by_list(link); if (r < 0) @@ -3436,6 +3439,7 @@ int link_initialized(Link *link, sd_device *device) { return 0; log_link_debug(link, "udev initialized link"); + link_set_state(link, LINK_STATE_INITIALIZED); link->sd_device = sd_device_ref(device); @@ -4353,6 +4357,7 @@ void link_clean(Link *link) { static const char* const link_state_table[_LINK_STATE_MAX] = { [LINK_STATE_PENDING] = "pending", + [LINK_STATE_INITIALIZED] = "initialized", [LINK_STATE_CONFIGURING] = "configuring", [LINK_STATE_CONFIGURED] = "configured", [LINK_STATE_UNMANAGED] = "unmanaged", diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index e65246c87d..e322ec2262 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -20,12 +20,13 @@ #include "set.h" typedef enum LinkState { - LINK_STATE_PENDING, - LINK_STATE_CONFIGURING, - LINK_STATE_CONFIGURED, - LINK_STATE_UNMANAGED, - LINK_STATE_FAILED, - LINK_STATE_LINGER, + LINK_STATE_PENDING, /* udev has not initialized the link */ + LINK_STATE_INITIALIZED, /* udev has initialized the link */ + LINK_STATE_CONFIGURING, /* configuring addresses, routes, etc. */ + LINK_STATE_CONFIGURED, /* everything is configured */ + LINK_STATE_UNMANAGED, /* Unmanaged=yes is set */ + LINK_STATE_FAILED, /* at least one configuration process failed */ + LINK_STATE_LINGER, /* RTM_DELLINK for the link has been received */ _LINK_STATE_MAX, _LINK_STATE_INVALID = -1 } LinkState; From 5f707e1280d7c66d3adcffd47a23ad446257f355 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Apr 2019 16:38:45 +0900 Subject: [PATCH 3/3] network: fix ref/unref logic for Link object - bridge or bonding master takes a reference of slave links. - drop link from bridge or bonding master's slave list when slave link is removed. - change type of Link::slaves to Set*, Fixes #12315. --- src/network/networkd-link.c | 109 +++++++++++++++++---------------- src/network/networkd-link.h | 2 +- src/network/networkd-manager.c | 7 +-- 3 files changed, 61 insertions(+), 57 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 3bc7f69cb3..d3928e2862 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -409,7 +409,7 @@ void link_update_operstate(Link *link, bool also_update_master) { if (operstate >= LINK_OPERSTATE_CARRIER) { Link *slave; - HASHMAP_FOREACH(slave, link->slaves, i) { + SET_FOREACH(slave, link->slaves, i) { link_update_operstate(slave, false); if (slave->operstate < LINK_OPERSTATE_CARRIER) @@ -605,17 +605,8 @@ static int link_new(Manager *manager, sd_netlink_message *message, Link **ret) { return 0; } -static void link_detach_from_manager(Link *link) { - if (!link || !link->manager) - return; - - hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex)); - set_remove(link->manager->links_requesting_uuid, link); - link_clean(link); -} - static Link *link_free(Link *link) { - Link *carrier, *master; + Link *carrier; Address *address; Route *route; Iterator i; @@ -663,10 +654,7 @@ static Link *link_free(Link *link) { sd_ndisc_unref(link->ndisc); sd_radv_unref(link->radv); - link_detach_from_manager(link); - free(link->ifname); - free(link->kind); (void) unlink(link->state_file); @@ -682,17 +670,7 @@ static Link *link_free(Link *link) { hashmap_remove(link->bound_by_links, INT_TO_PTR(carrier->ifindex)); hashmap_free(link->bound_by_links); - hashmap_free(link->slaves); - - if (link->network) { - if (link->network->bond && - link_get(link->manager, link->network->bond->ifindex, &master) >= 0) - (void) hashmap_remove(master->slaves, INT_TO_PTR(link->ifindex)); - - if (link->network->bridge && - link_get(link->manager, link->network->bridge->ifindex, &master) >= 0) - (void) hashmap_remove(master->slaves, INT_TO_PTR(link->ifindex)); - } + set_free_with_destructor(link->slaves, link_unref); return mfree(link); } @@ -1751,28 +1729,6 @@ static int link_set_bond(Link *link) { return r; } -static int link_append_to_master(Link *link, NetDev *netdev) { - Link *master; - int r; - - assert(link); - assert(netdev); - - r = link_get(link->manager, netdev->ifindex, &master); - if (r < 0) - return r; - - r = hashmap_ensure_allocated(&master->slaves, NULL); - if (r < 0) - return r; - - r = hashmap_put(master->slaves, INT_TO_PTR(link->ifindex), link); - if (r < 0) - return r; - - return 0; -} - static int link_lldp_save(Link *link) { _cleanup_free_ char *temp_path = NULL; _cleanup_fclose_ FILE *f = NULL; @@ -2535,6 +2491,55 @@ static void link_free_carrier_maps(Link *link) { return; } +static int link_append_to_master(Link *link, NetDev *netdev) { + Link *master; + int r; + + assert(link); + assert(netdev); + + r = link_get(link->manager, netdev->ifindex, &master); + if (r < 0) + return r; + + r = set_ensure_allocated(&master->slaves, NULL); + if (r < 0) + return r; + + r = set_put(master->slaves, link); + if (r < 0) + return r; + + link_ref(link); + return 0; +} + +static void link_drop_from_master(Link *link, NetDev *netdev) { + Link *master; + + assert(link); + + if (!link->manager || !netdev) + return; + + if (link_get(link->manager, netdev->ifindex, &master) < 0) + return; + + link_unref(set_remove(master->slaves, link)); +} + +static void link_detach_from_manager(Link *link) { + if (!link || !link->manager) + return; + + link_unref(set_remove(link->manager->links_requesting_uuid, link)); + link_clean(link); + + /* The following must be called at last. */ + assert_se(hashmap_remove(link->manager->links, INT_TO_PTR(link->ifindex)) == link); + link_unref(link); +} + void link_drop(Link *link) { if (!link || link->state == LINK_STATE_LINGER) return; @@ -2543,15 +2548,15 @@ void link_drop(Link *link) { link_free_carrier_maps(link); + if (link->network) { + link_drop_from_master(link, link->network->bridge); + link_drop_from_master(link, link->network->bond); + } + log_link_debug(link, "Link removed"); (void) unlink(link->state_file); - link_detach_from_manager(link); - - link_unref(link); - - return; } static int link_joined(Link *link) { diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index e322ec2262..e5497379d7 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -122,7 +122,7 @@ typedef struct Link { Hashmap *bound_by_links; Hashmap *bound_to_links; - Hashmap *slaves; + Set *slaves; } Link; typedef int (*link_netlink_message_handler_t)(sd_netlink*, sd_netlink_message*, Link*); diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 677f66a478..c9579370cb 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1450,13 +1450,12 @@ void manager_free(Manager *m) { } m->dirty_links = set_free_with_destructor(m->dirty_links, link_unref); - m->links = hashmap_free(m->links); - m->links_requesting_uuid = set_free(m->links_requesting_uuid); - set_free(m->duids_requesting_uuid); + m->links_requesting_uuid = set_free_with_destructor(m->links_requesting_uuid, link_unref); + m->links = hashmap_free_with_destructor(m->links, link_unref); + m->duids_requesting_uuid = set_free(m->duids_requesting_uuid); while ((network = m->networks)) network_free(network); - hashmap_free(m->networks_by_name); m->netdevs = hashmap_free_with_destructor(m->netdevs, netdev_unref);