From 2dba874c5ac6fc37d6562bed387254176e7674c7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 6 May 2022 08:03:39 +0200 Subject: [PATCH] dhcp: rename/refactor nm_dhcp_client_set_state() to be notifications Optimally we want stateless, pure code. Obviously, NMDhcpClient needs to keep state to know what it's doing. However, we should well encapsulate the state inside NMDhcpClient, and only accept events/notifications that mutate the internal state according to certain rules. Having a function public set_state(self, new_state) means that other components (subclasses of NMDhcpClient) can directly mangle the state. That means, you no longer need to only reason about the internal state of NMDhcpClient (and the events/notifications/state-changes that it implements). You also need to reason that other components take part of maintaining that internal state. Rename nm_dhcp_client_set_state() to nm_dhcp_client_notify(). Also, add a new enum NMDhcpClientEventType with notification/event types. In practice, this is only renaming. But naming is important, because it suggests the reader how to think about the code. (cherry picked from commit 97e65e4b50852710144b9b61f111e33981ab4a0f) --- src/core/dhcp/nm-dhcp-client.c | 70 ++++++++++++++++++++++++++------ src/core/dhcp/nm-dhcp-client.h | 17 +++++++- src/core/dhcp/nm-dhcp-nettools.c | 15 +++---- src/core/dhcp/nm-dhcp-systemd.c | 25 ++++++------ 4 files changed, 94 insertions(+), 33 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index f742537355..78fcb8ae36 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -244,23 +244,41 @@ _no_lease_timeout_schedule(NMDhcpClient *self) /*****************************************************************************/ void -nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3ConfigData *l3cd) +_nm_dhcp_client_notify(NMDhcpClient *self, + NMDhcpClientEventType client_event_type, + const NML3ConfigData *l3cd) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); GHashTable *options; const int IS_IPv4 = NM_IS_IPv4(priv->config.addr_family); nm_auto_unref_l3cd const NML3ConfigData *l3cd_merged = NULL; - if (NM_IN_SET(new_state, NM_DHCP_STATE_BOUND, NM_DHCP_STATE_EXTENDED)) { - nm_assert(NM_IS_L3_CONFIG_DATA(l3cd)); - nm_assert(nm_l3_config_data_get_dhcp_lease(l3cd, priv->config.addr_family)); - } else - nm_assert(!l3cd); + nm_assert(NM_IN_SET(client_event_type, + NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED, + NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED, + NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT, + NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE, + NM_DHCP_CLIENT_EVENT_TYPE_FAIL, + NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED)); + nm_assert((client_event_type >= NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT) + == NM_IN_SET(client_event_type, + NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT, + NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE, + NM_DHCP_CLIENT_EVENT_TYPE_FAIL, + NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED)); + nm_assert((!!l3cd) + == NM_IN_SET(client_event_type, + NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED)); + + nm_assert(!l3cd || NM_IS_L3_CONFIG_DATA(l3cd)); + nm_assert(!l3cd || nm_l3_config_data_get_dhcp_lease(l3cd, priv->config.addr_family)); if (l3cd) nm_l3_config_data_seal(l3cd); - if (new_state >= NM_DHCP_STATE_TIMEOUT) + if (client_event_type >= NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT) watch_cleanup(self); if (!IS_IPv4 && l3cd) { @@ -283,7 +301,7 @@ nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3Co } /* FIXME(l3cfg:dhcp): the API of NMDhcpClient is changing to expose a simpler API. - * The internals like NMDhcpState should not be exposed (or possibly dropped in large + * The internals like the state should not be exposed (or possibly dropped in large * parts). */ nm_l3_config_data_reset(&priv->l3cd, l3cd); @@ -344,7 +362,8 @@ nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3Co * as a static address (bypassing ACD), then NML3Cfg is aware of that and signals * immediate success. */ - if (nm_dhcp_client_can_accept(self) && new_state == NM_DHCP_STATE_BOUND && priv->l3cd + if (nm_dhcp_client_can_accept(self) && client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND + && priv->l3cd && nm_l3_config_data_get_num_addresses(priv->l3cd, priv->config.addr_family) > 0) { priv->l3cfg_notify.wait_dhcp_commit = TRUE; } else { @@ -381,7 +400,7 @@ daemon_watch_cb(GPid pid, int status, gpointer user_data) priv->pid = -1; - nm_dhcp_client_set_state(self, NM_DHCP_STATE_TERMINATED, NULL); + _nm_dhcp_client_notify(self, NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED, NULL); } void @@ -748,7 +767,7 @@ nm_dhcp_client_stop(NMDhcpClient *self, gboolean release) _LOGI("canceled DHCP transaction"); nm_assert(priv->pid == -1); - nm_dhcp_client_set_state(self, NM_DHCP_STATE_TERMINATED, NULL); + _nm_dhcp_client_notify(self, NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED, NULL); } /*****************************************************************************/ @@ -882,6 +901,7 @@ nm_dhcp_client_handle_event(gpointer unused, { NMDhcpClientPrivate *priv; nm_auto_unref_l3cd_init NML3ConfigData *l3cd = NULL; + NMDhcpClientEventType client_event_type; NMDhcpState new_state; NMPlatformIP6Address prefix = { 0, @@ -988,7 +1008,33 @@ nm_dhcp_client_handle_event(gpointer unused, new_state = NM_DHCP_STATE_UNKNOWN; } - nm_dhcp_client_set_state(self, new_state, l3cd); + switch (new_state) { + case NM_DHCP_STATE_UNKNOWN: + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED; + break; + case NM_DHCP_STATE_BOUND: + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_BOUND; + break; + case NM_DHCP_STATE_EXTENDED: + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED; + break; + case NM_DHCP_STATE_TIMEOUT: + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT; + break; + case NM_DHCP_STATE_EXPIRE: + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE; + break; + case NM_DHCP_STATE_FAIL: + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; + break; + case NM_DHCP_STATE_TERMINATED: + case NM_DHCP_STATE_DONE: + case NM_DHCP_STATE_NOOP: + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED; + break; + } + + _nm_dhcp_client_notify(self, client_event_type, l3cd); return TRUE; } diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index b172a72a44..95b2868759 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -38,6 +38,18 @@ typedef enum { NM_DHCP_STATE_TERMINATED, /* client is no longer running */ } NMDhcpState; +typedef enum { + NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED, + + NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED, + + NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT, + NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE, + NM_DHCP_CLIENT_EVENT_TYPE_FAIL, + NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED, +} NMDhcpClientEventType; + typedef enum _nm_packed { NM_DHCP_CLIENT_NOTIFY_TYPE_LEASE_UPDATE, @@ -268,8 +280,9 @@ void nm_dhcp_client_watch_child(NMDhcpClient *self, pid_t pid); void nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid); -void -nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3ConfigData *l3cd); +void _nm_dhcp_client_notify(NMDhcpClient *self, + NMDhcpClientEventType client_event_type, + const NML3ConfigData *l3cd); gboolean nm_dhcp_client_handle_event(gpointer unused, const char *iface, diff --git a/src/core/dhcp/nm-dhcp-nettools.c b/src/core/dhcp/nm-dhcp-nettools.c index aac189676d..185fa04ff5 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -796,15 +796,16 @@ bound4_handle(NMDhcpNettools *self, NDhcp4ClientLease *lease, gboolean extended) if (!l3cd) { _LOGW("failure to parse lease: %s", error->message); g_clear_error(&error); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return; } lease_save(self, lease, priv->lease_file); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), - extended ? NM_DHCP_STATE_EXTENDED : NM_DHCP_STATE_BOUND, - l3cd); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), + extended ? NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED + : NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + l3cd); } static void @@ -841,10 +842,10 @@ dhcp4_event_handle(NMDhcpNettools *self, NDhcp4ClientEvent *event) break; case N_DHCP4_CLIENT_EVENT_RETRACTED: case N_DHCP4_CLIENT_EVENT_EXPIRED: - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_EXPIRE, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE, NULL); break; case N_DHCP4_CLIENT_EVENT_CANCELLED: - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); break; case N_DHCP4_CLIENT_EVENT_GRANTED: priv->lease = n_dhcp4_client_lease_ref(event->granted.lease); @@ -896,7 +897,7 @@ dhcp4_event_cb(int fd, GIOCondition condition, gpointer user_data) */ _LOGE("error %d dispatching events", r); nm_clear_g_source_inst(&priv->event_source); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return G_SOURCE_REMOVE; } diff --git a/src/core/dhcp/nm-dhcp-systemd.c b/src/core/dhcp/nm-dhcp-systemd.c index 4a718de9ec..f2dd1823a8 100644 --- a/src/core/dhcp/nm-dhcp-systemd.c +++ b/src/core/dhcp/nm-dhcp-systemd.c @@ -459,7 +459,7 @@ bound4_handle(NMDhcpSystemd *self, gboolean extended) if (sd_dhcp_client_get_lease(priv->client4, &lease) < 0 || !lease) { _LOGW("no lease!"); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return; } @@ -473,15 +473,16 @@ bound4_handle(NMDhcpSystemd *self, gboolean extended) if (!l3cd) { _LOGW("%s", error->message); g_clear_error(&error); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return; } dhcp_lease_save(lease, priv->lease_file); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), - extended ? NM_DHCP_STATE_EXTENDED : NM_DHCP_STATE_BOUND, - l3cd); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), + extended ? NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED + : NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + l3cd); } static int @@ -500,10 +501,10 @@ dhcp_event_cb(sd_dhcp_client *client, int event, gpointer user_data) switch (event) { case SD_DHCP_CLIENT_EVENT_EXPIRED: - nm_dhcp_client_set_state(NM_DHCP_CLIENT(user_data), NM_DHCP_STATE_EXPIRE, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(user_data), NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE, NULL); break; case SD_DHCP_CLIENT_EVENT_STOP: - nm_dhcp_client_set_state(NM_DHCP_CLIENT(user_data), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(user_data), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); break; case SD_DHCP_CLIENT_EVENT_RENEW: case SD_DHCP_CLIENT_EVENT_IP_CHANGE: @@ -876,7 +877,7 @@ bound6_handle(NMDhcpSystemd *self) if (sd_dhcp6_client_get_lease(priv->client6, &lease) < 0 || !lease) { _LOGW(" no lease!"); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return; } @@ -892,11 +893,11 @@ bound6_handle(NMDhcpSystemd *self) if (!l3cd) { _LOGW("%s", error->message); - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); return; } - nm_dhcp_client_set_state(NM_DHCP_CLIENT(self), NM_DHCP_STATE_BOUND, l3cd); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(self), NM_DHCP_CLIENT_EVENT_TYPE_BOUND, l3cd); sd_dhcp6_lease_reset_pd_prefix_iter(lease); while (!sd_dhcp6_lease_get_pd(lease, @@ -921,11 +922,11 @@ dhcp6_event_cb(sd_dhcp6_client *client, int event, gpointer user_data) switch (event) { case SD_DHCP6_CLIENT_EVENT_RETRANS_MAX: - nm_dhcp_client_set_state(NM_DHCP_CLIENT(user_data), NM_DHCP_STATE_TIMEOUT, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(user_data), NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT, NULL); break; case SD_DHCP6_CLIENT_EVENT_RESEND_EXPIRE: case SD_DHCP6_CLIENT_EVENT_STOP: - nm_dhcp_client_set_state(NM_DHCP_CLIENT(user_data), NM_DHCP_STATE_FAIL, NULL); + _nm_dhcp_client_notify(NM_DHCP_CLIENT(user_data), NM_DHCP_CLIENT_EVENT_TYPE_FAIL, NULL); break; case SD_DHCP6_CLIENT_EVENT_IP_ACQUIRE: case SD_DHCP6_CLIENT_EVENT_INFORMATION_REQUEST: