diff options
author | Beniamino Galvani <bgalvani@redhat.com> | 2022-01-18 13:28:58 +0100 |
---|---|---|
committer | Beniamino Galvani <bgalvani@redhat.com> | 2022-01-26 14:54:51 +0100 |
commit | e1648d0665a0a2f619f0d63053b8a7e5b4fa0df8 (patch) | |
tree | e297ea1373374366eab25438348a4d720b89bc89 | |
parent | 62b2aa85e875d9c842f878cac896dd1fa7c5d988 (diff) |
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.
-rw-r--r-- | src/core/devices/nm-device.c | 95 | ||||
-rw-r--r-- | src/core/dhcp/nm-dhcp-client.c | 124 | ||||
-rw-r--r-- | src/core/dhcp/nm-dhcp-client.h | 1 |
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, ¬ify_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; |