Merge pull request #31247 from yuwata/network-make-reload-bus-method-synchronous

network: make Reload bus method synchronous

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),

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"

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;
 

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);

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);
 }
 

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;

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);

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)

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)"