core: commit l3cd asynchronously on DHCP bound event

When a lease is obtained, currently NMDevice performs a synchronous
commit of IP configuration and then accepts the lease.

Instead, let NMDevice only schedule a async commit; when the DHCP
client notices that the new address was committed it will
automatically accept it and emit a new signal so that the device can
succeed the activation.

Sync commits should be avoided because a commit does of things which
are outside the control of the caller (see the comment in
nm_device_l3cfg_commit()). Furthermore, when there are many pending
activations, async commits seem to help in reducing the CPU usage.

While making the commit async, also move the responsibility of the
accept() to NMDhcpClient.

3 files changed, 150 insertions, 70 deletions

diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c
index 3786ba211f..3bb78c4b4a 100644
--- a/src/core/devices/nm-device.c
+++ b/src/core/devices/nm-device.c
@@ -815,8 +815,6 @@ static void _dev_ipdhcpx_set_state(NMDevice *self, int addr_family, NMDeviceIPSt
 
 static void _dev_ipdhcpx_restart(NMDevice *self, int addr_family, gboolean release);
 
-static void _dev_ipdhcpx_handle_accept(NMDevice *self, int addr_family, const NML3ConfigData *l3cd);
-
 static gboolean
 _dev_ipac6_grace_period_start(NMDevice *self, guint32 timeout_sec, gboolean force_restart);
 
@@ -9916,63 +9914,6 @@ _dev_ipdhcpx_handle_fail(NMDevice *self, int addr_family, const char *reason)
 }
 
 static void
-_dev_ipdhcpx_handle_accept(NMDevice *self, int addr_family, const NML3ConfigData *l3cd)
-{
-    NMDevicePrivate      *priv    = NM_DEVICE_GET_PRIVATE(self);
-    const int             IS_IPv4 = NM_IS_IPv4(addr_family);
-    gs_free_error GError *error   = NULL;
-
-    nm_assert(NM_IS_L3_CONFIG_DATA(l3cd));
-
-    nm_dhcp_config_set_lease(priv->ipdhcp_data_x[IS_IPv4].config, l3cd);
-    _dev_l3_register_l3cds_set_one_full(self,
-                                        L3_CONFIG_DATA_TYPE_DHCP_X(IS_IPv4),
-                                        l3cd,
-                                        NM_L3CFG_CONFIG_FLAGS_FORCE_ONCE,
-                                        TRUE);
-
-    /* FIXME(l3cfg:dhcp): accept also should be handled by NMDhcpClient transparently.
-     * NMDhcpClient should do ACD (if enabled), and only after that passes, it exposes
-     * the lease to the user (NMDevice). NMDevice then may choose to configure the address.
-     * NMDhcpClient then checks in NM_L3_CONFIG_NOTIFY_TYPE_POST_COMMIT whether the requested
-     * address is to be configured by NML3Cfg (here, the intent is what matters, not
-     * whether the address is actually visible in NMPlatform -- that is because while NML3Cfg
-     * configures the address (in platform), the user might delete it right away. Also,
-     * depending on the commit type, NML3Cfg may even choose not to configure it right now
-     * (which arguably will be odd). Anyway, what matters is whether the user configured
-     * the address in NML3Cfg (by checking the ObjStateData).
-     *
-     * If yes, then NMDhcpClient needs to accept automatically.
-     *
-     * Doing it here is wrong for two reasons:
-     *
-     * - NMDevice already has enough to do, it should not be concerned with telling
-     *   NMDhcpClient to accept the lease, it should only configure the address.
-     *
-     * - we should only accept the lease *after* configuring the address (see n_dhcp4_client_lease_accept().
-     *   Currently this only works by passing commit_sync=TRUE to _dev_l3_register_l3cds_set_one(), but
-     *   doing that is wrong (see FIXME why commit_sync needs to go away). */
-    if (priv->ipdhcp_data_x[IS_IPv4].state != NM_DEVICE_IP_STATE_READY
-        && !nm_dhcp_client_accept(priv->ipdhcp_data_x[IS_IPv4].client, &error)) {
-        _LOGW(LOGD_DHCPX(IS_IPv4), "error accepting lease: %s", error->message);
-        _dev_ipdhcpx_set_state(self, addr_family, NM_DEVICE_IP_STATE_FAILED);
-        _dev_ip_state_check_async(self, addr_family);
-        return;
-    }
-
-    _dev_ipdhcpx_set_state(self, addr_family, NM_DEVICE_IP_STATE_READY);
-
-    nm_dispatcher_call_device(NM_DISPATCHER_ACTION_DHCP_CHANGE_X(IS_IPv4),
-                              self,
-                              NULL,
-                              NULL,
-                              NULL,
-                              NULL);
-
-    _dev_ip_state_check_async(self, addr_family);
-}
-
-static void
 _dev_ipdhcpx_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_data, NMDevice *self)
 {
     NMDevicePrivate *priv        = NM_DEVICE_GET_PRIVATE(self);
@@ -10015,8 +9956,32 @@ _dev_ipdhcpx_notify(NMDhcpClient *client, const NMDhcpClientNotifyData *notify_d
             return;
         }
 
+        if (notify_data->lease_update.accepted) {
+            _LOGT_ipdhcp(addr_family, "lease accepted");
+            if (priv->ipdhcp_data_x[IS_IPv4].state != NM_DEVICE_IP_STATE_READY) {
+                _dev_ipdhcpx_set_state(self, addr_family, NM_DEVICE_IP_STATE_READY);
+                nm_dispatcher_call_device(NM_DISPATCHER_ACTION_DHCP_CHANGE_X(IS_IPv4),
+                                          self,
+                                          NULL,
+                                          NULL,
+                                          NULL,
+                                          NULL);
+                _dev_ip_state_check_async(self, addr_family);
+            }
+            return;
+        }
+
+        /* Schedule a commit of the configuration. The DHCP client
+         * will accept the lease once the address is committed, and
+         * will send a LEASE_UPDATE notification with accepted=1. */
         _LOGT_ipdhcp(addr_family, "lease update");
-        _dev_ipdhcpx_handle_accept(self, addr_family, notify_data->lease_update.l3cd);
+        nm_dhcp_config_set_lease(priv->ipdhcp_data_x[IS_IPv4].config,
+                                 notify_data->lease_update.l3cd);
+        _dev_l3_register_l3cds_set_one_full(self,
+                                            L3_CONFIG_DATA_TYPE_DHCP_X(IS_IPv4),
+                                            notify_data->lease_update.l3cd,
+                                            NM_L3CFG_CONFIG_FLAGS_FORCE_ONCE,
+                                            FALSE);
         return;
     }
 
@@ -10213,8 +10178,14 @@ _dev_ipdhcpx_start(NMDevice *self, int addr_family)
         priv->ipdhcp_data_x[IS_IPv4].config = nm_dhcp_config_new(addr_family, previous_lease);
         _notify(self, PROP_DHCPX_CONFIG(IS_IPv4));
     }
-    if (previous_lease)
-        _dev_ipdhcpx_handle_accept(self, addr_family, previous_lease);
+    if (previous_lease) {
+        nm_dhcp_config_set_lease(priv->ipdhcp_data_x[IS_IPv4].config, previous_lease);
+        _dev_l3_register_l3cds_set_one_full(self,
+                                            L3_CONFIG_DATA_TYPE_DHCP_X(IS_IPv4),
+                                            previous_lease,
+                                            NM_L3CFG_CONFIG_FLAGS_FORCE_ONCE,
+                                            FALSE);
+    }
 
     return;
 
diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c
index f7c095fb8f..54f9ee59fd 100644
--- a/src/core/dhcp/nm-dhcp-client.c
+++ b/src/core/dhcp/nm-dhcp-client.c
@@ -43,12 +43,17 @@ typedef struct _NMDhcpClientPrivate {
     const NML3ConfigData *l3cd;
     GSource              *no_lease_timeout_source;
     GSource              *ipv6_lladdr_timeout_source;
+    GBytes               *effective_client_id;
     pid_t                 pid;
     guint                 watch_id;
     NMDhcpState           state;
     bool                  iaid_explicit : 1;
     bool                  is_stopped : 1;
-    GBytes               *effective_client_id;
+    struct {
+        gulong id;
+        bool   wait_dhcp_commit : 1;
+        bool   wait_ll_address : 1;
+    } l3cfg_notify;
 } NMDhcpClientPrivate;
 
 G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT)
@@ -57,6 +62,11 @@ G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT)
 
 /*****************************************************************************/
 
+static void
+l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcpClient *self);
+
+/*****************************************************************************/
+
 /* we use pid=-1 for invalid PIDs. Ensure that pid_t can hold negative values. */
 G_STATIC_ASSERT(!(((pid_t) -1) > 0));
 
@@ -70,6 +80,27 @@ _emit_notify(NMDhcpClient *self, const NMDhcpClientNotifyData *notify_data)
 
 /*****************************************************************************/
 
+static void
+connect_l3cfg_notify(NMDhcpClient *self)
+{
+    NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self);
+    gboolean             do_connect;
+
+    do_connect = priv->l3cfg_notify.wait_dhcp_commit | priv->l3cfg_notify.wait_ll_address;
+
+    if (!do_connect) {
+        nm_clear_g_signal_handler(priv->config.l3cfg, &priv->l3cfg_notify.id);
+        return;
+    }
+
+    if (priv->l3cfg_notify.id == 0) {
+        priv->l3cfg_notify.id = g_signal_connect(priv->config.l3cfg,
+                                                 NM_L3CFG_SIGNAL_NOTIFY,
+                                                 G_CALLBACK(l3_cfg_notify_cb),
+                                                 self);
+    }
+}
+
 pid_t
 nm_dhcp_client_get_pid(NMDhcpClient *self)
 {
@@ -357,6 +388,9 @@ nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3Co
                 },
         };
 
+        priv->l3cfg_notify.wait_dhcp_commit = (new_state == NM_DHCP_STATE_BOUND);
+        connect_l3cfg_notify(self);
+
         _emit_notify(self, &notify_data);
     }
 }
@@ -523,14 +557,20 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp
 
     nm_assert(l3cfg == priv->config.l3cfg);
 
-    if (notify_data->notify_type == NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE) {
+    switch (notify_data->notify_type) {
+    case NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE:
+    {
         const NMPlatformIP6Address *addr;
         gs_free_error GError       *error = NULL;
 
+        if (!priv->l3cfg_notify.wait_ll_address)
+            return;
+
         addr = ipv6_lladdr_find(self);
         if (addr) {
             _LOGD("got IPv6LL address, starting transaction");
-            g_signal_handlers_disconnect_by_func(l3cfg, l3_cfg_notify_cb, self);
+            priv->l3cfg_notify.wait_ll_address = FALSE;
+            connect_l3cfg_notify(self);
             nm_clear_g_source_inst(&priv->ipv6_lladdr_timeout_source);
 
             schedule_no_lease_timeout(self);
@@ -543,6 +583,74 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp
                              }));
             }
         }
+
+        break;
+    }
+    case NM_L3_CONFIG_NOTIFY_TYPE_POST_COMMIT:
+    {
+        const NML3ConfigData      *committed_l3cd;
+        NMDedupMultiIter           ipconf_iter;
+        const NMPlatformIPAddress *lease_address;
+        gs_free_error GError      *error = NULL;
+
+        /* A new configuration was committed to the interface. If we previously
+         * got a lease, check whether we are waiting for the address to be
+         * configured. If the address was added, we can proceed accepting the
+         * lease and notifying NMDevice. */
+
+        if (!priv->l3cfg_notify.wait_dhcp_commit)
+            return;
+
+        nm_l3_config_data_iter_ip_address_for_each (&ipconf_iter,
+                                                    priv->l3cd,
+                                                    priv->config.addr_family,
+                                                    &lease_address)
+            break;
+        nm_assert(lease_address);
+        committed_l3cd = nm_l3cfg_get_combined_l3cd(l3cfg, TRUE);
+
+        if (priv->config.addr_family == AF_INET) {
+            const NMPlatformIP4Address *address4 = (const NMPlatformIP4Address *) lease_address;
+
+            if (!nm_l3_config_data_lookup_address_4(committed_l3cd,
+                                                    address4->address,
+                                                    address4->plen,
+                                                    address4->peer_address))
+                return;
+        } else {
+            const NMPlatformIP6Address *address6 = (const NMPlatformIP6Address *) lease_address;
+
+            if (!nm_l3_config_data_lookup_address_6(committed_l3cd, &address6->address))
+                return;
+        }
+
+        priv->l3cfg_notify.wait_dhcp_commit = FALSE;
+        connect_l3cfg_notify(self);
+
+        _LOGD("accept address");
+
+        if (!nm_dhcp_client_accept(self, &error)) {
+            gs_free char *reason = g_strdup_printf("error accepting lease: %s", error->message);
+
+            _emit_notify(self,
+                         &((NMDhcpClientNotifyData){
+                             .notify_type         = NM_DHCP_CLIENT_NOTIFY_TYPE_IT_LOOKS_BAD,
+                             .it_looks_bad.reason = reason,
+                         }));
+            return;
+        }
+
+        _emit_notify(
+            self,
+            &((NMDhcpClientNotifyData){.notify_type  = NM_DHCP_CLIENT_NOTIFY_TYPE_LEASE_UPDATE,
+                                       .lease_update = {
+                                           .l3cd     = priv->l3cd,
+                                           .accepted = TRUE,
+                                       }}));
+        break;
+    };
+    default:
+        /* ignore */;
     }
 }
 
@@ -569,10 +677,8 @@ nm_dhcp_client_start_ip6(NMDhcpClient *self, GError **error)
     addr = ipv6_lladdr_find(self);
     if (!addr) {
         _LOGD("waiting for IPv6LL address");
-        g_signal_connect(priv->config.l3cfg,
-                         NM_L3CFG_SIGNAL_NOTIFY,
-                         G_CALLBACK(l3_cfg_notify_cb),
-                         self);
+        priv->l3cfg_notify.wait_ll_address = TRUE;
+        connect_l3cfg_notify(self);
         priv->ipv6_lladdr_timeout_source =
             nm_g_timeout_add_seconds_source(10, ipv6_lladdr_timeout, self);
         return TRUE;
@@ -657,7 +763,9 @@ nm_dhcp_client_stop(NMDhcpClient *self, gboolean release)
 
     priv->is_stopped = TRUE;
 
-    g_signal_handlers_disconnect_by_func(priv->config.l3cfg, l3_cfg_notify_cb, self);
+    priv->l3cfg_notify.wait_dhcp_commit = FALSE;
+    priv->l3cfg_notify.wait_ll_address  = FALSE;
+    connect_l3cfg_notify(self);
 
     /* Kill the DHCP client */
     old_pid = priv->pid;
diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h
index d8f659fa4a..249bd013ff 100644
--- a/src/core/dhcp/nm-dhcp-client.h
+++ b/src/core/dhcp/nm-dhcp-client.h
@@ -71,6 +71,7 @@ typedef struct {
              * or NULL (if a previous lease timed out). It can also be the
              * previous lease, that was injected. */
             const NML3ConfigData *l3cd;
+            bool                  accepted;
         } lease_update;
         struct {
             const NMPlatformIP6Address *prefix;