From 7d55b1348bf6e0a5810e21723eca8678b02eb162 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 15:37:55 +0200 Subject: [PATCH] dhcp: don't pass duid to client ip6_start() and stop() We don't do that for ip4_start() either. The duid/client-id is stored inside the NMDhcpClient instance, and the function can access it from there. Maybe, it is often preferable to have stateless objects and not relying on ip4_start() to obtain the client ID from the client's state. However, the purpose of the NMDhcpClient object is to hold state about DHCP. To simplify the complexity of objects that inherrently have state, we should be careful about mutating the state. It adds little additional complexity of only reading the state when needed anyway. In fact, it adds complexity, because previously it wasn't enough to check all callers of nm_dhcp_client_get_client_id() to see where the client-id is used. Instead, one would also need to follow the @duid argument several layers of the call stack. --- src/dhcp/nm-dhcp-client.c | 11 +++++------ src/dhcp/nm-dhcp-client.h | 4 +--- src/dhcp/nm-dhcp-dhclient.c | 13 +++++-------- src/dhcp/nm-dhcp-dhcpcanon.c | 6 +++--- src/dhcp/nm-dhcp-dhcpcd.c | 5 ++--- src/dhcp/nm-dhcp-systemd.c | 14 ++++++++------ 6 files changed, 24 insertions(+), 29 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index 870615b9fa..e47a50d5b2 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -365,7 +365,7 @@ nm_dhcp_client_stop_pid (pid_t pid, const char *iface) } static void -stop (NMDhcpClient *self, gboolean release, GBytes *duid) +stop (NMDhcpClient *self, gboolean release) { NMDhcpClientPrivate *priv; @@ -557,14 +557,14 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, gs_unref_bytes GBytes *own_client_id = NULL; g_return_val_if_fail (NM_IS_DHCP_CLIENT (self), FALSE); + g_return_val_if_fail (client_id, FALSE); priv = NM_DHCP_CLIENT_GET_PRIVATE (self); + g_return_val_if_fail (priv->pid == -1, FALSE); g_return_val_if_fail (priv->addr_family == AF_INET6, FALSE); g_return_val_if_fail (priv->uuid != NULL, FALSE); - - nm_assert (!priv->client_id); - nm_assert (client_id); + g_return_val_if_fail (!priv->client_id, FALSE); if (!enforce_duid) own_client_id = NM_DHCP_CLIENT_GET_CLASS (self)->get_duid (self); @@ -585,7 +585,6 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, dhcp_anycast_addr, ll_addr, privacy, - priv->client_id, needed_prefixes, error); } @@ -652,7 +651,7 @@ nm_dhcp_client_stop (NMDhcpClient *self, gboolean release) /* Kill the DHCP client */ old_pid = priv->pid; - NM_DHCP_CLIENT_GET_CLASS (self)->stop (self, release, priv->client_id); + NM_DHCP_CLIENT_GET_CLASS (self)->stop (self, release); if (old_pid > 0) _LOGI ("canceled DHCP transaction, DHCP client pid %d", old_pid); else diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index 0b97534a18..0d2aaaaad9 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -86,13 +86,11 @@ typedef struct { const char *anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error); void (*stop) (NMDhcpClient *self, - gboolean release, - GBytes *duid); + gboolean release); /** * get_duid: diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index 9e8dd6a5f3..468cbc8440 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -339,7 +339,6 @@ create_dhclient_config (NMDhcpDhclient *self, static gboolean dhclient_start (NMDhcpClient *client, const char *mode_opt, - GBytes *duid, gboolean release, pid_t *out_pid, int prefixes, @@ -418,7 +417,9 @@ dhclient_start (NMDhcpClient *client, /* Save the DUID to the leasefile dhclient will actually use */ if (addr_family == AF_INET6) { - if (!nm_dhcp_dhclient_save_duid (priv->lease_file, duid, &local)) { + if (!nm_dhcp_dhclient_save_duid (priv->lease_file, + nm_dhcp_client_get_client_id (client), + &local)) { nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, "failed to save DUID to '%s': %s", @@ -537,7 +538,6 @@ ip4_start (NMDhcpClient *client, nm_dhcp_client_set_client_id (client, new_client_id); } return dhclient_start (client, - NULL, NULL, FALSE, NULL, @@ -550,7 +550,6 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { @@ -578,7 +577,6 @@ ip6_start (NMDhcpClient *client, nm_dhcp_client_get_info_only (NM_DHCP_CLIENT (self)) ? "-S" : "-N", - duid, FALSE, NULL, needed_prefixes, @@ -586,12 +584,12 @@ ip6_start (NMDhcpClient *client, } static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpDhclient *self = NM_DHCP_DHCLIENT (client); NMDhcpDhclientPrivate *priv = NM_DHCP_DHCLIENT_GET_PRIVATE (self); - NM_DHCP_CLIENT_CLASS (nm_dhcp_dhclient_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_dhclient_parent_class)->stop (client, release); if (priv->conf_file) if (remove (priv->conf_file) == -1) @@ -610,7 +608,6 @@ stop (NMDhcpClient *client, gboolean release, GBytes *duid) if (dhclient_start (client, NULL, - duid, TRUE, &rpid, 0, diff --git a/src/dhcp/nm-dhcp-dhcpcanon.c b/src/dhcp/nm-dhcp-dhcpcanon.c index acd3ce2d4c..0f033e2250 100644 --- a/src/dhcp/nm-dhcp-dhcpcanon.c +++ b/src/dhcp/nm-dhcp-dhcpcanon.c @@ -193,20 +193,20 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "dhcpcanon plugin does not support IPv6"); return FALSE; } + static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpDhcpcanon *self = NM_DHCP_DHCPCANON (client); NMDhcpDhcpcanonPrivate *priv = NM_DHCP_DHCPCANON_GET_PRIVATE (self); - NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcanon_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcanon_parent_class)->stop (client, release); if (priv->pid_file) { if (remove (priv->pid_file) == -1) diff --git a/src/dhcp/nm-dhcp-dhcpcd.c b/src/dhcp/nm-dhcp-dhcpcd.c index 98ab5342ce..e2a1354f2c 100644 --- a/src/dhcp/nm-dhcp-dhcpcd.c +++ b/src/dhcp/nm-dhcp-dhcpcd.c @@ -187,7 +187,6 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { @@ -196,12 +195,12 @@ ip6_start (NMDhcpClient *client, } static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpDhcpcd *self = NM_DHCP_DHCPCD (client); NMDhcpDhcpcdPrivate *priv = NM_DHCP_DHCPCD_GET_PRIVATE (self); - NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcd_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcd_parent_class)->stop (client, release); if (priv->pid_file) { if (remove (priv->pid_file) == -1) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 5b7b5fbeeb..aacfc13229 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -895,7 +895,6 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { @@ -907,10 +906,13 @@ ip6_start (NMDhcpClient *client, int r, i; const guint8 *duid_arr; gsize duid_len; + GBytes *duid; - g_assert (priv->client4 == NULL); - g_assert (priv->client6 == NULL); - g_return_val_if_fail (duid != NULL, FALSE); + g_return_val_if_fail (!priv->client4, FALSE); + g_return_val_if_fail (!priv->client6, FALSE); + + duid = nm_dhcp_client_get_client_id (client); + g_return_val_if_fail (duid, FALSE); duid_arr = g_bytes_get_data (duid, &duid_len); if (!duid_arr || duid_len < 2) @@ -1013,13 +1015,13 @@ errout: } static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpSystemd *self = NM_DHCP_SYSTEMD (client); NMDhcpSystemdPrivate *priv = NM_DHCP_SYSTEMD_GET_PRIVATE (self); int r = 0; - NM_DHCP_CLIENT_CLASS (nm_dhcp_systemd_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_systemd_parent_class)->stop (client, release); _LOGT ("dhcp-client%d: stop %p", priv->client4 ? '4' : '6',