From 1e869a5de9063dfe6afac52bffc7c9c2d206b096 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 8 Feb 2024 12:47:39 +0900 Subject: [PATCH 1/2] network: make Reload bus method synchronous Prompted by https://github.com/systemd/systemd/pull/30085#discussion_r1401534107. Note, like Reconfigure bus method, even reconfiguration for an interface is triggered by Reload method, the method only wait for the link enters configuring state (or unmanaged state if no matching .network file exists). Users still need to invoke systemd-networkd-wait-online if it is necessary to wait for the interface enters configured state after Reload medhod. --- src/libsystemd/sd-bus/bus-common-errors.c | 1 + src/libsystemd/sd-bus/bus-common-errors.h | 1 + src/network/networkd-link.c | 74 +++++++++++++++++++++++ src/network/networkd-link.h | 1 + src/network/networkd-manager-bus.c | 10 ++- src/network/networkd-manager.c | 15 +++-- src/network/networkd-manager.h | 4 +- 7 files changed, 98 insertions(+), 8 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-common-errors.c b/src/libsystemd/sd-bus/bus-common-errors.c index df26fd75cd..efdd6539cc 100644 --- a/src/libsystemd/sd-bus/bus-common-errors.c +++ b/src/libsystemd/sd-bus/bus-common-errors.c @@ -110,6 +110,7 @@ BUS_ERROR_MAP_ELF_REGISTER const sd_bus_error_map bus_common_errors[] = { SD_BUS_ERROR_MAP(BUS_ERROR_SPEED_METER_INACTIVE, EOPNOTSUPP), SD_BUS_ERROR_MAP(BUS_ERROR_UNMANAGED_INTERFACE, EOPNOTSUPP), + SD_BUS_ERROR_MAP(BUS_ERROR_NETWORK_ALREADY_RELOADING, EBUSY), SD_BUS_ERROR_MAP(BUS_ERROR_NO_SUCH_HOME, EEXIST), SD_BUS_ERROR_MAP(BUS_ERROR_UID_IN_USE, EEXIST), diff --git a/src/libsystemd/sd-bus/bus-common-errors.h b/src/libsystemd/sd-bus/bus-common-errors.h index 3a0eef49ef..2961ee4a9e 100644 --- a/src/libsystemd/sd-bus/bus-common-errors.h +++ b/src/libsystemd/sd-bus/bus-common-errors.h @@ -111,6 +111,7 @@ #define BUS_ERROR_SPEED_METER_INACTIVE "org.freedesktop.network1.SpeedMeterInactive" #define BUS_ERROR_UNMANAGED_INTERFACE "org.freedesktop.network1.UnmanagedInterface" +#define BUS_ERROR_NETWORK_ALREADY_RELOADING "org.freedesktop.network1.AlreadyReloading" #define BUS_ERROR_NO_SUCH_HOME "org.freedesktop.home1.NoSuchHome" #define BUS_ERROR_UID_IN_USE "org.freedesktop.home1.UIDInUse" diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 621f859206..c0d0f83ca3 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -1463,6 +1463,80 @@ int link_reconfigure(Link *link, bool force) { return 1; /* 1 means the interface will be reconfigured. */ } +typedef struct ReconfigureData { + Link *link; + Manager *manager; + sd_bus_message *message; +} ReconfigureData; + +static void reconfigure_data_destroy_callback(ReconfigureData *data) { + int r; + + assert(data); + assert(data->link); + assert(data->manager); + assert(data->manager->reloading > 0); + assert(data->message); + + link_unref(data->link); + + data->manager->reloading--; + if (data->manager->reloading <= 0) { + r = sd_bus_reply_method_return(data->message, NULL); + if (r < 0) + log_warning_errno(r, "Failed to send reply for 'Reload' DBus method, ignoring: %m"); + } + + sd_bus_message_unref(data->message); + free(data); +} + +static int reconfigure_handler_on_bus_method_reload(sd_netlink *rtnl, sd_netlink_message *m, ReconfigureData *data) { + assert(data); + assert(data->link); + return link_reconfigure_handler_internal(rtnl, m, data->link, /* force = */ false); +} + +int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message) { + _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL; + _cleanup_free_ ReconfigureData *data = NULL; + int r; + + assert(link); + assert(link->manager); + assert(link->manager->rtnl); + assert(message); + + /* See comments in link_reconfigure() above. */ + if (IN_SET(link->state, LINK_STATE_PENDING, LINK_STATE_INITIALIZED, LINK_STATE_LINGER)) + return 0; + + r = sd_rtnl_message_new_link(link->manager->rtnl, &req, RTM_GETLINK, link->ifindex); + if (r < 0) + return r; + + data = new(ReconfigureData, 1); + if (!data) + return -ENOMEM; + + r = netlink_call_async(link->manager->rtnl, NULL, req, + reconfigure_handler_on_bus_method_reload, + reconfigure_data_destroy_callback, data); + if (r < 0) + return r; + + *data = (ReconfigureData) { + .link = link_ref(link), + .manager = link->manager, + .message = sd_bus_message_ref(message), + }; + + link->manager->reloading++; + + TAKE_PTR(data); + return 0; +} + static int link_initialized_and_synced(Link *link) { int r; diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index d81b45bd64..0167b1988d 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -252,6 +252,7 @@ LinkState link_state_from_string(const char *s) _pure_; int link_reconfigure_impl(Link *link, bool force); int link_reconfigure(Link *link, bool force); +int link_reconfigure_on_bus_method_reload(Link *link, sd_bus_message *message); int manager_udev_process_link(Manager *m, sd_device *device, sd_device_action_t action); int manager_rtnl_process_link(sd_netlink *rtnl, sd_netlink_message *message, Manager *m); diff --git a/src/network/networkd-manager-bus.c b/src/network/networkd-manager-bus.c index 035537c869..3c3d815a4a 100644 --- a/src/network/networkd-manager-bus.c +++ b/src/network/networkd-manager-bus.c @@ -198,9 +198,12 @@ static int bus_method_reconfigure_link(sd_bus_message *message, void *userdata, } static int bus_method_reload(sd_bus_message *message, void *userdata, sd_bus_error *error) { - Manager *manager = userdata; + Manager *manager = ASSERT_PTR(userdata); int r; + if (manager->reloading > 0) + return sd_bus_error_set(error, BUS_ERROR_NETWORK_ALREADY_RELOADING, "Already reloading."); + r = bus_verify_polkit_async( message, "org.freedesktop.network1.reload", @@ -212,10 +215,13 @@ static int bus_method_reload(sd_bus_message *message, void *userdata, sd_bus_err if (r == 0) return 1; /* Polkit will call us back */ - r = manager_reload(manager); + r = manager_reload(manager, message); if (r < 0) return r; + if (manager->reloading > 0) + return 1; /* Will reply later. */ + return sd_bus_reply_method_return(message, NULL); } diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 6f3d90d64c..42c6371be5 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -453,7 +453,7 @@ static int signal_restart_callback(sd_event_source *s, const struct signalfd_sig static int signal_reload_callback(sd_event_source *s, const struct signalfd_siginfo *si, void *userdata) { Manager *m = ASSERT_PTR(userdata); - manager_reload(m); + (void) manager_reload(m, /* message = */ NULL); return 0; } @@ -1085,7 +1085,7 @@ int manager_set_timezone(Manager *m, const char *tz) { return 0; } -int manager_reload(Manager *m) { +int manager_reload(Manager *m, sd_bus_message *message) { Link *link; int r; @@ -1105,9 +1105,14 @@ int manager_reload(Manager *m) { goto finish; HASHMAP_FOREACH(link, m->links_by_index) { - r = link_reconfigure(link, /* force = */ false); - if (r < 0) - goto finish; + if (message) + r = link_reconfigure_on_bus_method_reload(link, message); + else + r = link_reconfigure(link, /* force = */ false); + if (r < 0) { + log_link_warning_errno(link, r, "Failed to reconfigure the interface: %m"); + link_enter_failed(link); + } } r = 0; diff --git a/src/network/networkd-manager.h b/src/network/networkd-manager.h index a97ae8ea21..7788ce7d6f 100644 --- a/src/network/networkd-manager.h +++ b/src/network/networkd-manager.h @@ -105,6 +105,8 @@ struct Manager { OrderedSet *remove_request_queue; Hashmap *tuntap_fds_by_name; + + unsigned reloading; }; int manager_new(Manager **ret, bool test_mode); @@ -125,6 +127,6 @@ int manager_enumerate(Manager *m); int manager_set_hostname(Manager *m, const char *hostname); int manager_set_timezone(Manager *m, const char *timezone); -int manager_reload(Manager *m); +int manager_reload(Manager *m, sd_bus_message *message); DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_free); From 4bc771d0614de18ca4d0b1312e5e12666c7e00d5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 8 Feb 2024 12:55:07 +0900 Subject: [PATCH 2/2] test: drop unnecessary sleep Now, 'Reload' dbus method is synchronous. It is not necessary to wait for link enter configuring state. --- test/test-network/systemd-networkd-tests.py | 6 +----- test/units/testsuite-74.networkctl.sh | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 1700732e34..41c4c55062 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -789,12 +789,8 @@ def networkctl_json(*args): def networkctl_reconfigure(*links): networkctl('reconfigure', *links) -def networkctl_reload(sleep_time=1): +def networkctl_reload(): networkctl('reload') - # 'networkctl reload' asynchronously reconfigure links. - # Hence, we need to wait for a short time for link to be in configuring state. - if sleep_time > 0: - time.sleep(sleep_time) def resolvectl(*args): return check_output(*(resolvectl_cmd + list(args)), env=env) diff --git a/test/units/testsuite-74.networkctl.sh b/test/units/testsuite-74.networkctl.sh index 06a3c39e77..6cd5267b72 100755 --- a/test/units/testsuite-74.networkctl.sh +++ b/test/units/testsuite-74.networkctl.sh @@ -104,7 +104,6 @@ networkctl cat @test2:network | cmp - <(networkctl cat "$NETWORK_NAME") EDITOR='cp' script -ec 'networkctl edit @test2 --drop-in test2.conf' /dev/null cmp "+4" "/etc/systemd/network/${NETWORK_NAME}.d/test2.conf" -sleep 1 (! EDITOR='true' script -ec 'networkctl edit @test2 --runtime --drop-in test2.conf' /dev/null) ip_link="$(ip link show test2)"