From 56e1f0d29065724d79caf52b06c8c96fe4be5e63 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 21:19:01 +0200 Subject: [PATCH 01/24] contrib: ignore error installing behave_html_formatter in container Yes, this is currently broken. *sigh*. Ignore any failure. --- contrib/scripts/nm-in-container.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contrib/scripts/nm-in-container.sh b/contrib/scripts/nm-in-container.sh index 02a16e9da3..5e1e24ca9f 100755 --- a/contrib/scripts/nm-in-container.sh +++ b/contrib/scripts/nm-in-container.sh @@ -327,8 +327,7 @@ RUN dnf install -y \\ RUN dnf debuginfo-install --skip-broken \$(ldd /usr/sbin/NetworkManager | sed -n 's/.* => \\(.*\\) (0x[0-9A-Fa-f]*)$/\1/p' | xargs -n1 readlink -f) -y -RUN pip3 install --user \\ - behave_html_formatter +RUN pip3 install --user behave_html_formatter || true RUN systemctl enable NetworkManager From 0ca6aaab329dd668ac8411aded47fd66a8c6eb2b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 21:24:19 +0200 Subject: [PATCH 02/24] contrib: don't use "Z" specifier for mounting base directory in "nm-in-container.sh" It seems unnecessary, and can cause failure. --- contrib/scripts/nm-in-container.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/scripts/nm-in-container.sh b/contrib/scripts/nm-in-container.sh index 5e1e24ca9f..4c71610b8a 100755 --- a/contrib/scripts/nm-in-container.sh +++ b/contrib/scripts/nm-in-container.sh @@ -411,7 +411,7 @@ do_run() { podman run --privileged \ --name "$CONTAINER_NAME_NAME" \ -d \ - -v "$BASEDIR_NM:$BASEDIR_NM:Z" \ + -v "$BASEDIR_NM:$BASEDIR_NM" \ "${BIND_NM_CI[@]}" \ "${BIND_FILES[@]}" \ "$CONTAINER_NAME_REPOSITORY:$CONTAINER_NAME_TAG" From a859cee560ccde429ffa94edc9300e68a4b986b8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 21:27:32 +0200 Subject: [PATCH 03/24] contrib: fix /etc/motd for "nm-in-container.sh" --- contrib/scripts/nm-in-container.sh | 94 +++++++++++++++--------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/contrib/scripts/nm-in-container.sh b/contrib/scripts/nm-in-container.sh index 4c71610b8a..95fc306a95 100755 --- a/contrib/scripts/nm-in-container.sh +++ b/contrib/scripts/nm-in-container.sh @@ -109,53 +109,53 @@ find NetworkManager bind mounted at $BASEDIR_NM run \`nm-env-prepare.sh setup --idx 1\` to setup test interfaces Configure NetworkManager with - \$ ./configure \ - --enable-address-sanitizer=no \ - --enable-compile-warnings=yes \ - --enable-concheck \ - --enable-config-plugin-ibft=yes \ - --enable-gtk-doc \ - --enable-ifcfg-rh=yes \ - --enable-ifcfg-suse \ - --enable-ifnet \ - --enable-ifupdown=yes \ - --enable-introspection \ - --enable-json-validation=yes \ - --enable-maintainer-mode \ - --enable-more-logging \ - --enable-more-warnings=error \ - --enable-ovs=yes \ - --enable-polkit=yes \ - --enable-teamdctl=yes \ - --enable-undefined-sanitizer=no \ - --enable-vala=yes \ - --enable-wimax \ - --localstatedir=/var \ - --prefix=/opt/test \ - --sysconfdir=/etc \ - --with-config-dhcp-default=internal \ - --with-config-dns-rc-manager-default=auto \ - --with-consolekit=yes \ - --with-consolekit=yes \ - --with-crypto=nss \ - --with-dhclient=yes \ - --with-dhcpcanon=yes \ - --with-dhcpcd=yes \ - --with-iwd=yes \ - --with-libnm-glib=yes \ - --with-modem-manager-1 \ - --with-netconfig=/bin/nowhere/netconfig \ - --with-nm-cloud-setup=yes \ - --with-nmcli=yes \ - --with-nmtui=yes \ - --with-ofono=yes \ - --with-resolvconf=/bin/nowhere/resolvconf \ - --with-session-tracking=systemd \ - --with-suspend-resume=systemd \ - --with-systemd-logind=yes \ - --with-valgrind=yes \ - --enable-tests="\${NM_BUILD_TESTS:-yes}" \ - --with-more-asserts="\${NM_BUILD_MORE_ASSERTS:-1000}" \ + \$ ./configure \\ + --enable-address-sanitizer=no \\ + --enable-compile-warnings=yes \\ + --enable-concheck \\ + --enable-config-plugin-ibft=yes \\ + --enable-gtk-doc \\ + --enable-ifcfg-rh=yes \\ + --enable-ifcfg-suse \\ + --enable-ifnet \\ + --enable-ifupdown=yes \\ + --enable-introspection \\ + --enable-json-validation=yes \\ + --enable-maintainer-mode \\ + --enable-more-logging \\ + --enable-more-warnings=error \\ + --enable-ovs=yes \\ + --enable-polkit=yes \\ + --enable-teamdctl=yes \\ + --enable-undefined-sanitizer=no \\ + --enable-vala=yes \\ + --enable-wimax \\ + --localstatedir=/var \\ + --prefix=/opt/test \\ + --sysconfdir=/etc \\ + --with-config-dhcp-default=internal \\ + --with-config-dns-rc-manager-default=auto \\ + --with-consolekit=yes \\ + --with-consolekit=yes \\ + --with-crypto=nss \\ + --with-dhclient=yes \\ + --with-dhcpcanon=yes \\ + --with-dhcpcd=yes \\ + --with-iwd=yes \\ + --with-libnm-glib=yes \\ + --with-modem-manager-1 \\ + --with-netconfig=/bin/nowhere/netconfig \\ + --with-nm-cloud-setup=yes \\ + --with-nmcli=yes \\ + --with-nmtui=yes \\ + --with-ofono=yes \\ + --with-resolvconf=/bin/nowhere/resolvconf \\ + --with-session-tracking=systemd \\ + --with-suspend-resume=systemd \\ + --with-systemd-logind=yes \\ + --with-valgrind=yes \\ + --enable-tests="\${NM_BUILD_TESTS:-yes}" \\ + --with-more-asserts="\${NM_BUILD_MORE_ASSERTS:-1000}" \\ "\${NM_CONFIGURE_OTPS[@]}" Test with: \$ systemctl stop NetworkManager; /opt/test/sbin/NetworkManager --debug 2>&1 | tee -a /tmp/nm-log.txt From 7d1a9b898d55b624b80d0175c1f7405ca64a14d5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 22:06:58 +0200 Subject: [PATCH 04/24] glib-aux/trivial: fix typo in code comment --- src/libnm-glib-aux/nm-ref-string.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-ref-string.h b/src/libnm-glib-aux/nm-ref-string.h index 3363bce00a..eb3c38de21 100644 --- a/src/libnm-glib-aux/nm-ref-string.h +++ b/src/libnm-glib-aux/nm-ref-string.h @@ -119,7 +119,7 @@ nm_ref_string_cmp(NMRefString *a, NMRefString *b) NM_CMP_SELF(a, b); /* It would be cheaper to first compare by length. But this - * way we get a nicer, ASCIIbethical sort order. */ + * way we get a nicer, ASCIIbetical sort order. */ NM_CMP_DIRECT_MEMCMP(a->str, b->str, NM_MIN(a->len, b->len)); NM_CMP_DIRECT(a->len, b->len); return nm_assert_unreachable_val(0); From 222f4049284160d1b55e79789224800d988724e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 22:00:16 +0200 Subject: [PATCH 05/24] glib-aux: add nm_g_bytes_ref() helper g_bytes_ref() does not accept NULL. But doing so can be convenient, add a helper for that. Note that g_bytes_unref() does accept NULL, so there is no corresponding helper. --- src/libnm-glib-aux/nm-shared-utils.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 0e123cf1c0..5552bf0567 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -623,6 +623,16 @@ nm_utils_is_separator(const char c) /*****************************************************************************/ +static inline GBytes * +nm_g_bytes_ref(GBytes *b) +{ + if (b) + g_bytes_ref(b); + return b; +} + +/*****************************************************************************/ + GBytes *nm_g_bytes_get_empty(void); GBytes *nm_g_bytes_new_from_str(const char *str); From 9b9c07530cc931c172be2dbf08742612ccc7b453 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 30 Apr 2022 21:05:58 +0200 Subject: [PATCH 06/24] dhcp: cleanup reason_to_state() in "nm-dhcp-client.c" - use NM_IN_STRSET_ASCII_CASE(). - don't use else block after we return. - don't accept the "iface" argument just for logging. The caller can do the logging, if they wish. --- src/core/dhcp/nm-dhcp-client.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index e71590858a..4124a590a8 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -154,30 +154,23 @@ NM_UTILS_LOOKUP_STR_DEFINE(nm_dhcp_state_to_string, NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_UNKNOWN, "unknown"), ); static NMDhcpState -reason_to_state(NMDhcpClient *self, const char *iface, const char *reason) +reason_to_state(const char *reason) { - if (g_ascii_strcasecmp(reason, "bound") == 0 || g_ascii_strcasecmp(reason, "bound6") == 0 - || g_ascii_strcasecmp(reason, "static") == 0) + if (NM_IN_STRSET_ASCII_CASE(reason, "bound", "bound6", "static")) return NM_DHCP_STATE_BOUND; - else if (g_ascii_strcasecmp(reason, "renew") == 0 || g_ascii_strcasecmp(reason, "renew6") == 0 - || g_ascii_strcasecmp(reason, "reboot") == 0 - || g_ascii_strcasecmp(reason, "rebind") == 0 - || g_ascii_strcasecmp(reason, "rebind6") == 0) + if (NM_IN_STRSET_ASCII_CASE(reason, "renew", "renew6", "reboot", "rebind", "rebind6")) return NM_DHCP_STATE_EXTENDED; - else if (g_ascii_strcasecmp(reason, "timeout") == 0) + if (NM_IN_STRSET_ASCII_CASE(reason, "timeout")) return NM_DHCP_STATE_TIMEOUT; - else if (g_ascii_strcasecmp(reason, "nak") == 0 || g_ascii_strcasecmp(reason, "expire") == 0 - || g_ascii_strcasecmp(reason, "expire6") == 0) + if (NM_IN_STRSET_ASCII_CASE(reason, "nak", "expire", "expire6")) return NM_DHCP_STATE_EXPIRE; - else if (g_ascii_strcasecmp(reason, "end") == 0 || g_ascii_strcasecmp(reason, "stop") == 0 - || g_ascii_strcasecmp(reason, "stopped") == 0) + if (NM_IN_STRSET_ASCII_CASE(reason, "end", "stop", "stopped")) return NM_DHCP_STATE_DONE; - else if (g_ascii_strcasecmp(reason, "fail") == 0 || g_ascii_strcasecmp(reason, "abend") == 0) + if (NM_IN_STRSET_ASCII_CASE(reason, "fail", "abend")) return NM_DHCP_STATE_FAIL; - else if (g_ascii_strcasecmp(reason, "preinit") == 0) + if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) return NM_DHCP_STATE_NOOP; - _LOGD("unmapped DHCP state '%s'", reason); return NM_DHCP_STATE_UNKNOWN; } @@ -915,7 +908,11 @@ nm_dhcp_client_handle_event(gpointer unused, if (priv->pid != pid) return FALSE; - new_state = reason_to_state(self, priv->config.iface, reason); + new_state = reason_to_state(reason); + + if (new_state == NM_DHCP_STATE_UNKNOWN) + _LOGD("unmapped DHCP state '%s'", reason); + if (new_state == NM_DHCP_STATE_NOOP) return TRUE; From 668d8050a50d20a1497042cc19d2400fdd0035d9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 30 Apr 2022 21:53:40 +0200 Subject: [PATCH 07/24] dhcp: cleanup bytearray_variant_to_string() - the code comment was unclear/wrong. If something comes from an environment variables it is *NOT* UTF-8 safe. Also, we convert all non-ASCII characters, not only non UTF-8 characters. - as we already convert the string to ASCII, the check whether it's UTF-8 is bogus. - using GString is unnecessary. --- src/core/dhcp/nm-dhcp-client.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 4124a590a8..52a46b6633 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -765,37 +765,31 @@ static char * bytearray_variant_to_string(NMDhcpClient *self, GVariant *value, const char *key) { const guint8 *array; + char *str; gsize length; - GString *str; - int i; - unsigned char c; - char *converted = NULL; + gsize i; - g_return_val_if_fail(value != NULL, NULL); + nm_assert(value); array = g_variant_get_fixed_array(value, &length, 1); - /* Since the DHCP options come through environment variables, they should - * already be UTF-8 safe, but just make sure. + /* Since the DHCP options come originally came as environment variables, they + * have not guaranteed encoding. Let's only accept ASCII here. */ - str = g_string_sized_new(length); + str = g_malloc(length + 1); for (i = 0; i < length; i++) { - c = array[i]; + guint8 c = array[i]; - /* Convert NULLs to spaces and non-ASCII characters to ? */ if (c == '\0') - c = ' '; + str[i] = ' '; else if (c > 127) - c = '?'; - str = g_string_append_c(str, c); + str[i] = '?'; + else + str[i] = (char) c; } - str = g_string_append_c(str, '\0'); + str[i] = '\0'; - converted = str->str; - if (!g_utf8_validate(converted, -1, NULL)) - _LOGW("option '%s' couldn't be converted to UTF-8", key); - g_string_free(str, FALSE); - return converted; + return str; } static int From cb2ab420a24a86ccc5efcdcbf9d4ae1c54a76ca7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 30 Apr 2022 22:36:08 +0200 Subject: [PATCH 08/24] dhcp: don't assert against untrusted data in maybe_add_option() --- src/core/dhcp/nm-dhcp-client.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 52a46b6633..888dc3f4e8 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -814,7 +814,8 @@ maybe_add_option(NMDhcpClient *self, GHashTable *hash, const char *key, GVariant { char *str_value = NULL; - g_return_if_fail(g_variant_is_of_type(value, G_VARIANT_TYPE_BYTESTRING)); + if (!g_variant_is_of_type(value, G_VARIANT_TYPE_BYTESTRING)) + return; if (g_str_has_prefix(key, OLD_TAG)) return; From 98f7081db2458909b905eec0d7ab10562bd7c75c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 30 Apr 2022 22:37:39 +0200 Subject: [PATCH 09/24] dhcp: minor cleanup in maybe_add_option() - return early to avoid nested block. - use NM_STR_HAS_PREFIX() over g_str_has_prefix(), because that can be inlined and only accepts a C literal as prefix argument. --- src/core/dhcp/nm-dhcp-client.c | 38 +++++++++++++++++----------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 888dc3f4e8..54f71cc8bc 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -812,12 +812,13 @@ label_is_unknown_xyz(const char *label) static void maybe_add_option(NMDhcpClient *self, GHashTable *hash, const char *key, GVariant *value) { - char *str_value = NULL; + char *str_value; + int priv_opt_num; if (!g_variant_is_of_type(value, G_VARIANT_TYPE_BYTESTRING)) return; - if (g_str_has_prefix(key, OLD_TAG)) + if (NM_STR_HAS_PREFIX(key, OLD_TAG)) return; /* Filter out stuff that's not actually new DHCP options */ @@ -830,34 +831,33 @@ maybe_add_option(NMDhcpClient *self, GHashTable *hash, const char *key, GVariant return; str_value = bytearray_variant_to_string(self, value, key); - if (str_value) { - int priv_opt_num; + if (!str_value) + return; - g_hash_table_insert(hash, g_strdup(key), str_value); + g_hash_table_insert(hash, g_strdup(key), str_value); - /* dhclient has no special labels for private dhcp options: it uses "unknown_xyz" + /* dhclient has no special labels for private dhcp options: it uses "unknown_xyz" * labels for that. We need to identify those to alias them to our "private_xyz" * format unused in the internal dchp plugins. */ - if ((priv_opt_num = label_is_unknown_xyz(key)) > 0) { - gs_free guint8 *check_val = NULL; - char *hex_str = NULL; - gsize len; + if ((priv_opt_num = label_is_unknown_xyz(key)) > 0) { + gs_free guint8 *check_val = NULL; + char *hex_str = NULL; + gsize len; - /* dhclient passes values from dhcp private options in its own "string" format: + /* dhclient passes values from dhcp private options in its own "string" format: * if the raw values are printable as ascii strings, it will pass the string * representation; if the values are not printable as an ascii string, it will * pass a string displaying the hex values (hex string). Try to enforce passing * always an hex string, converting string representation if needed. */ - check_val = nm_utils_hexstr2bin_alloc(str_value, FALSE, TRUE, ":", 0, &len); - hex_str = nm_utils_bin2hexstr_full(check_val ?: (guint8 *) str_value, - check_val ? len : strlen(str_value), - ':', - FALSE, - NULL); - g_hash_table_insert(hash, g_strdup_printf("private_%d", priv_opt_num), hex_str); - } + check_val = nm_utils_hexstr2bin_alloc(str_value, FALSE, TRUE, ":", 0, &len); + hex_str = nm_utils_bin2hexstr_full(check_val ?: (guint8 *) str_value, + check_val ? len : strlen(str_value), + ':', + FALSE, + NULL); + g_hash_table_insert(hash, g_strdup_printf("private_%d", priv_opt_num), hex_str); } } From c8542a5d50b92786ae62cdb5c17e130947d2e3ec Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 21:52:23 +0200 Subject: [PATCH 10/24] dhcp: use GSource for watching child process instead of numeric source id --- src/core/dhcp/nm-dhcp-client.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 54f71cc8bc..40ceafd830 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -43,9 +43,9 @@ typedef struct _NMDhcpClientPrivate { const NML3ConfigData *l3cd; GSource *no_lease_timeout_source; GSource *ipv6_lladdr_timeout_source; + GSource *watch_source; GBytes *effective_client_id; pid_t pid; - guint watch_id; NMDhcpState state; bool iaid_explicit : 1; bool is_stopped : 1; @@ -181,7 +181,7 @@ watch_cleanup(NMDhcpClient *self) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - nm_clear_g_source(&priv->watch_id); + nm_clear_g_source_inst(&priv->watch_source); } void @@ -382,8 +382,9 @@ daemon_watch_cb(GPid pid, int status, gpointer user_data) NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); gs_free char *desc = NULL; - g_return_if_fail(priv->watch_id); - priv->watch_id = 0; + g_return_if_fail(priv->watch_source); + + priv->watch_source = NULL; _LOGI("client pid %d %s", pid, (desc = nm_utils_get_process_exit_status_desc(status))); @@ -400,8 +401,8 @@ nm_dhcp_client_watch_child(NMDhcpClient *self, pid_t pid) g_return_if_fail(priv->pid == -1); priv->pid = pid; - g_return_if_fail(priv->watch_id == 0); - priv->watch_id = g_child_watch_add(pid, daemon_watch_cb, self); + g_return_if_fail(!priv->watch_source); + priv->watch_source = nm_g_child_watch_add_source(pid, daemon_watch_cb, self); } void From 1093e6677606aaeafc422821bd25b2e4ff239247 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 22:01:15 +0200 Subject: [PATCH 11/24] dhcp: minor code cleanups in "nm-dhcp-client.c" --- src/core/dhcp/nm-dhcp-client.c | 49 +++++++++++++++++----------------- src/core/dhcp/nm-dhcp-utils.c | 3 +-- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 40ceafd830..c747659618 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -32,7 +32,10 @@ /*****************************************************************************/ -enum { SIGNAL_NOTIFY, LAST_SIGNAL }; +enum { + SIGNAL_NOTIFY, + LAST_SIGNAL, +}; static guint signals[LAST_SIGNAL] = {0}; @@ -112,7 +115,8 @@ nm_dhcp_client_get_pid(NMDhcpClient *self) void nm_dhcp_client_set_effective_client_id(NMDhcpClient *self, GBytes *client_id) { - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + gs_free char *tmp_str = NULL; g_return_if_fail(NM_IS_DHCP_CLIENT(self)); g_return_if_fail(!client_id || g_bytes_get_size(client_id) >= 2); @@ -123,19 +127,13 @@ nm_dhcp_client_set_effective_client_id(NMDhcpClient *self, GBytes *client_id) return; g_bytes_unref(priv->effective_client_id); - priv->effective_client_id = client_id; - if (client_id) - g_bytes_ref(client_id); + priv->effective_client_id = nm_g_bytes_ref(client_id); - { - gs_free char *s = NULL; - - _LOGT("%s: set %s", - priv->config.addr_family == AF_INET6 ? "duid" : "client-id", - priv->effective_client_id - ? (s = nm_dhcp_utils_duid_to_string(priv->effective_client_id)) - : "default"); - } + _LOGT("%s: set %s", + priv->config.addr_family == AF_INET6 ? "duid" : "client-id", + priv->effective_client_id + ? (tmp_str = nm_dhcp_utils_duid_to_string(priv->effective_client_id)) + : "default"); } /*****************************************************************************/ @@ -218,6 +216,8 @@ stop(NMDhcpClient *self, gboolean release) priv->pid = -1; } +/*****************************************************************************/ + static gboolean _no_lease_timeout(gpointer user_data) { @@ -230,6 +230,7 @@ _no_lease_timeout(gpointer user_data) &((NMDhcpClientNotifyData){ .notify_type = NM_DHCP_CLIENT_NOTIFY_TYPE_NO_LEASE_TIMEOUT, })); + return G_SOURCE_CONTINUE; } @@ -242,7 +243,7 @@ nm_dhcp_client_get_config(NMDhcpClient *self) } static void -schedule_no_lease_timeout(NMDhcpClient *self) +_no_lease_timeout_schedule(NMDhcpClient *self) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); @@ -260,6 +261,8 @@ schedule_no_lease_timeout(NMDhcpClient *self) } } +/*****************************************************************************/ + void nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3ConfigData *l3cd) { @@ -268,13 +271,11 @@ nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3Co const int IS_IPv4 = NM_IS_IPv4(priv->config.addr_family); nm_auto_unref_l3cd const NML3ConfigData *l3cd_merged = NULL; - g_return_if_fail(NM_IS_DHCP_CLIENT(self)); - if (NM_IN_SET(new_state, NM_DHCP_STATE_BOUND, NM_DHCP_STATE_EXTENDED)) { - g_return_if_fail(NM_IS_L3_CONFIG_DATA(l3cd)); - g_return_if_fail(nm_l3_config_data_get_dhcp_lease(l3cd, priv->config.addr_family)); + nm_assert(NM_IS_L3_CONFIG_DATA(l3cd)); + nm_assert(nm_l3_config_data_get_dhcp_lease(l3cd, priv->config.addr_family)); } else - g_return_if_fail(!l3cd); + nm_assert(!l3cd); if (l3cd) nm_l3_config_data_seal(l3cd); @@ -296,7 +297,7 @@ nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3Co nm_clear_g_source_inst(&priv->no_lease_timeout_source); } else { if (priv->l3cd) - schedule_no_lease_timeout(self); + _no_lease_timeout_schedule(self); } /* FIXME(l3cfg:dhcp): the API of NMDhcpClient is changing to expose a simpler API. @@ -428,7 +429,7 @@ nm_dhcp_client_start_ip4(NMDhcpClient *self, GError **error) g_return_val_if_fail(priv->config.addr_family == AF_INET, FALSE); g_return_val_if_fail(priv->config.uuid, FALSE); - schedule_no_lease_timeout(self); + _no_lease_timeout_schedule(self); return NM_DHCP_CLIENT_GET_CLASS(self)->ip4_start(self, error); } @@ -554,7 +555,7 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp connect_l3cfg_notify(self); nm_clear_g_source_inst(&priv->ipv6_lladdr_timeout_source); - schedule_no_lease_timeout(self); + _no_lease_timeout_schedule(self); if (!NM_DHCP_CLIENT_GET_CLASS(self)->ip6_start(self, &addr->address, &error)) { _emit_notify(self, @@ -665,7 +666,7 @@ nm_dhcp_client_start_ip6(NMDhcpClient *self, GError **error) return TRUE; } - schedule_no_lease_timeout(self); + _no_lease_timeout_schedule(self); return NM_DHCP_CLIENT_GET_CLASS(self)->ip6_start(self, &addr->address, error); } diff --git a/src/core/dhcp/nm-dhcp-utils.c b/src/core/dhcp/nm-dhcp-utils.c index a7c5ee2747..79310dc5cc 100644 --- a/src/core/dhcp/nm-dhcp-utils.c +++ b/src/core/dhcp/nm-dhcp-utils.c @@ -837,8 +837,7 @@ nm_dhcp_utils_merge_new_dhcp6_lease(const NML3ConfigData *l3cd_old, const char *start; const char *iaid; - nm_assert(out_l3cd_merged); - nm_assert(!*out_l3cd_merged); + nm_assert(out_l3cd_merged && !*out_l3cd_merged); if (!l3cd_old) return FALSE; From 8d121b17b57ab995726a0b081fde31b5bfbaf70f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 4 May 2022 22:21:44 +0200 Subject: [PATCH 12/24] dhcp: move code in "nm-dhcp-client.c" So that it makes more sense, related parts are closer together. --- src/core/dhcp/nm-dhcp-client.c | 144 +++++++++++++++++---------------- 1 file changed, 73 insertions(+), 71 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index c747659618..fbf388cc23 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -75,69 +75,6 @@ G_STATIC_ASSERT(!(((pid_t) -1) > 0)); /*****************************************************************************/ -static void -_emit_notify(NMDhcpClient *self, const NMDhcpClientNotifyData *notify_data) -{ - g_signal_emit(G_OBJECT(self), signals[SIGNAL_NOTIFY], 0, 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) -{ - g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), -1); - - return NM_DHCP_CLIENT_GET_PRIVATE(self)->pid; -} - -void -nm_dhcp_client_set_effective_client_id(NMDhcpClient *self, GBytes *client_id) -{ - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - gs_free char *tmp_str = NULL; - - g_return_if_fail(NM_IS_DHCP_CLIENT(self)); - g_return_if_fail(!client_id || g_bytes_get_size(client_id) >= 2); - - priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - - if (nm_g_bytes_equal0(priv->effective_client_id, client_id)) - return; - - g_bytes_unref(priv->effective_client_id); - priv->effective_client_id = nm_g_bytes_ref(client_id); - - _LOGT("%s: set %s", - priv->config.addr_family == AF_INET6 ? "duid" : "client-id", - priv->effective_client_id - ? (tmp_str = nm_dhcp_utils_duid_to_string(priv->effective_client_id)) - : "default"); -} - -/*****************************************************************************/ - NM_UTILS_LOOKUP_STR_DEFINE(nm_dhcp_state_to_string, NMDhcpState, NM_UTILS_LOOKUP_DEFAULT(NULL), @@ -174,6 +111,79 @@ reason_to_state(const char *reason) /*****************************************************************************/ +static void +_emit_notify(NMDhcpClient *self, const NMDhcpClientNotifyData *notify_data) +{ + g_signal_emit(G_OBJECT(self), signals[SIGNAL_NOTIFY], 0, 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); + } +} + +/*****************************************************************************/ + +void +nm_dhcp_client_set_effective_client_id(NMDhcpClient *self, GBytes *client_id) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + gs_free char *tmp_str = NULL; + + g_return_if_fail(NM_IS_DHCP_CLIENT(self)); + g_return_if_fail(!client_id || g_bytes_get_size(client_id) >= 2); + + priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + if (nm_g_bytes_equal0(priv->effective_client_id, client_id)) + return; + + g_bytes_unref(priv->effective_client_id); + priv->effective_client_id = nm_g_bytes_ref(client_id); + + _LOGT("%s: set %s", + priv->config.addr_family == AF_INET6 ? "duid" : "client-id", + priv->effective_client_id + ? (tmp_str = nm_dhcp_utils_duid_to_string(priv->effective_client_id)) + : "default"); +} + +const NMDhcpClientConfig * +nm_dhcp_client_get_config(NMDhcpClient *self) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + return &priv->config; +} + +/*****************************************************************************/ + +pid_t +nm_dhcp_client_get_pid(NMDhcpClient *self) +{ + g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), -1); + + return NM_DHCP_CLIENT_GET_PRIVATE(self)->pid; +} + static void watch_cleanup(NMDhcpClient *self) { @@ -234,14 +244,6 @@ _no_lease_timeout(gpointer user_data) return G_SOURCE_CONTINUE; } -const NMDhcpClientConfig * -nm_dhcp_client_get_config(NMDhcpClient *self) -{ - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - - return &priv->config; -} - static void _no_lease_timeout_schedule(NMDhcpClient *self) { From 70cbf3dc1ef2c068410574b8c4e791edba45cb70 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 May 2022 18:41:02 +0200 Subject: [PATCH 13/24] dhcp/trivial: add comment about nm_dhcp_utils_merge_new_dhcp6_lease() --- src/core/dhcp/nm-dhcp-client.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index fbf388cc23..ebdf25b388 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -286,6 +286,9 @@ nm_dhcp_client_set_state(NMDhcpClient *self, NMDhcpState new_state, const NML3Co watch_cleanup(self); if (!IS_IPv4 && l3cd) { + /* nm_dhcp_utils_merge_new_dhcp6_lease() relies on "life_starts" option + * for merging, which is only set by dhclient. Internal client never sets that, + * but it supports multiple IP addresses per lease. */ if (nm_dhcp_utils_merge_new_dhcp6_lease(priv->l3cd, l3cd, &l3cd_merged)) { _LOGD("lease merged with existing one"); l3cd = nm_l3_config_data_seal(l3cd_merged); From f102051a29836c3dc5b876e591b31f21a79cfa81 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 5 May 2022 21:18:00 +0200 Subject: [PATCH 14/24] dhcp: drop most of NMDhcpState usage from nm_dhcp_client_handle_event() NMDhcpState is very tied to events from dhclient. But most of these states we don't care about, and NMDhcpClient definitely should abstract and hide them. We should repurpose NMDhcpState to simpler state. For that, first drop the state from nm_dhcp_client_handle_event(). This is only the first step (which arguably makes the code more complicated, because reason_to_state() gets spread out and the logic happens more than once). That will be addressed next. --- src/core/dhcp/nm-dhcp-client.c | 69 ++++++++++++++++------------------ 1 file changed, 33 insertions(+), 36 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index ebdf25b388..7875a04adf 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -49,7 +49,6 @@ typedef struct _NMDhcpClientPrivate { GSource *watch_source; GBytes *effective_client_id; pid_t pid; - NMDhcpState state; bool iaid_explicit : 1; bool is_stopped : 1; struct { @@ -88,27 +87,6 @@ NM_UTILS_LOOKUP_STR_DEFINE(nm_dhcp_state_to_string, NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_TIMEOUT, "timeout"), NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_UNKNOWN, "unknown"), ); -static NMDhcpState -reason_to_state(const char *reason) -{ - if (NM_IN_STRSET_ASCII_CASE(reason, "bound", "bound6", "static")) - return NM_DHCP_STATE_BOUND; - if (NM_IN_STRSET_ASCII_CASE(reason, "renew", "renew6", "reboot", "rebind", "rebind6")) - return NM_DHCP_STATE_EXTENDED; - if (NM_IN_STRSET_ASCII_CASE(reason, "timeout")) - return NM_DHCP_STATE_TIMEOUT; - if (NM_IN_STRSET_ASCII_CASE(reason, "nak", "expire", "expire6")) - return NM_DHCP_STATE_EXPIRE; - if (NM_IN_STRSET_ASCII_CASE(reason, "end", "stop", "stopped")) - return NM_DHCP_STATE_DONE; - if (NM_IN_STRSET_ASCII_CASE(reason, "fail", "abend")) - return NM_DHCP_STATE_FAIL; - if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) - return NM_DHCP_STATE_NOOP; - - return NM_DHCP_STATE_UNKNOWN; -} - /*****************************************************************************/ static void @@ -891,11 +869,12 @@ nm_dhcp_client_handle_event(gpointer unused, NMDhcpClient *self) { NMDhcpClientPrivate *priv; - guint32 new_state; - nm_auto_unref_l3cd_init NML3ConfigData *l3cd = NULL; + nm_auto_unref_l3cd_init NML3ConfigData *l3cd = NULL; + NMDhcpState new_state; NMPlatformIP6Address prefix = { 0, }; + gboolean reason_is_bound; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); g_return_val_if_fail(iface != NULL, FALSE); @@ -910,21 +889,22 @@ nm_dhcp_client_handle_event(gpointer unused, if (priv->pid != pid) return FALSE; - new_state = reason_to_state(reason); + _LOGD("DHCP event (reason: '%s')", reason); - if (new_state == NM_DHCP_STATE_UNKNOWN) - _LOGD("unmapped DHCP state '%s'", reason); - - if (new_state == NM_DHCP_STATE_NOOP) + if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) return TRUE; - _LOGD("DHCP state '%s' -> '%s' (reason: '%s')", - nm_dhcp_state_to_string(priv->state), - nm_dhcp_state_to_string(new_state), - reason); - priv->state = new_state; + reason_is_bound = NM_IN_STRSET_ASCII_CASE(reason, + "bound", + "bound6", + "static", + "renew", + "renew6", + "reboot", + "rebind", + "rebind6"); - if (NM_IN_SET(new_state, NM_DHCP_STATE_BOUND, NM_DHCP_STATE_EXTENDED)) { + if (reason_is_bound) { gs_unref_hashtable GHashTable *str_options = NULL; GVariantIter iter; const char *name; @@ -974,9 +954,26 @@ nm_dhcp_client_handle_event(gpointer unused, } /* Fail if no valid IP config was received */ - if (NM_IN_SET(new_state, NM_DHCP_STATE_BOUND, NM_DHCP_STATE_EXTENDED) && !l3cd) { + if (reason_is_bound && !l3cd) { _LOGW("client bound but IP config not received"); new_state = NM_DHCP_STATE_FAIL; + } else { + if (NM_IN_STRSET_ASCII_CASE(reason, "bound", "bound6", "static")) + new_state = NM_DHCP_STATE_BOUND; + else if (NM_IN_STRSET_ASCII_CASE(reason, "renew", "renew6", "reboot", "rebind", "rebind6")) + new_state = NM_DHCP_STATE_EXTENDED; + else if (NM_IN_STRSET_ASCII_CASE(reason, "timeout")) + new_state = NM_DHCP_STATE_TIMEOUT; + else if (NM_IN_STRSET_ASCII_CASE(reason, "nak", "expire", "expire6")) + new_state = NM_DHCP_STATE_EXPIRE; + else if (NM_IN_STRSET_ASCII_CASE(reason, "end", "stop", "stopped")) + new_state = NM_DHCP_STATE_DONE; + else if (NM_IN_STRSET_ASCII_CASE(reason, "fail", "abend")) + new_state = NM_DHCP_STATE_FAIL; + else if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) + new_state = NM_DHCP_STATE_NOOP; + else + new_state = NM_DHCP_STATE_UNKNOWN; } nm_dhcp_client_set_state(self, new_state, l3cd); From 9761e38f7eeb50d33ee103dccae2cdc7d25cbd4b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 11:10:56 +0200 Subject: [PATCH 15/24] dhcp: fix handling of NM_DHCP_STATE_NOOP The "noop" state is almost unused, however, nm_dhcp_set_state() has a check "if (new_state >= NM_DHCP_STATE_TIMEOUT)", so the order of the NOOP state matters. Fix that by reordering. Also, just return right away from NOOP. --- src/core/dhcp/nm-dhcp-client.c | 2 +- src/core/dhcp/nm-dhcp-client.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 7875a04adf..157e6950d3 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -971,7 +971,7 @@ nm_dhcp_client_handle_event(gpointer unused, else if (NM_IN_STRSET_ASCII_CASE(reason, "fail", "abend")) new_state = NM_DHCP_STATE_FAIL; else if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) - new_state = NM_DHCP_STATE_NOOP; + return TRUE; else new_state = NM_DHCP_STATE_UNKNOWN; } diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index 249bd013ff..b172a72a44 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -28,6 +28,7 @@ typedef enum { NM_DHCP_STATE_UNKNOWN = 0, + NM_DHCP_STATE_NOOP, /* state is a non operation for NetworkManager */ NM_DHCP_STATE_BOUND, /* new lease */ NM_DHCP_STATE_EXTENDED, /* lease extended */ NM_DHCP_STATE_TIMEOUT, /* timed out contacting server */ @@ -35,7 +36,6 @@ typedef enum { NM_DHCP_STATE_EXPIRE, /* lease expired or NAKed */ NM_DHCP_STATE_FAIL, /* failed for some reason */ NM_DHCP_STATE_TERMINATED, /* client is no longer running */ - NM_DHCP_STATE_NOOP, /* state is a non operation for NetworkManager */ } NMDhcpState; typedef enum _nm_packed { From 97e65e4b50852710144b9b61f111e33981ab4a0f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 6 May 2022 08:03:39 +0200 Subject: [PATCH 16/24] 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. --- 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 | 10 ++--- 4 files changed, 86 insertions(+), 26 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 157e6950d3..3c0c748725 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) { @@ -284,7 +302,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); @@ -337,7 +355,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 { @@ -374,7 +393,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 @@ -741,7 +760,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); } /*****************************************************************************/ @@ -870,6 +889,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, @@ -976,7 +996,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 724442e6c4..b86b889d47 100644 --- a/src/core/dhcp/nm-dhcp-nettools.c +++ b/src/core/dhcp/nm-dhcp-nettools.c @@ -882,15 +882,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 @@ -927,10 +928,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); @@ -985,7 +986,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 045d528791..b106db2c60 100644 --- a/src/core/dhcp/nm-dhcp-systemd.c +++ b/src/core/dhcp/nm-dhcp-systemd.c @@ -205,7 +205,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; } @@ -221,11 +221,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, @@ -250,11 +250,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: From 802f343d9fac9829f968dc6c0a0270f7e7854b99 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 10:47:38 +0200 Subject: [PATCH 17/24] dhcp: drop NMDhcpState enum It's unused now. --- src/core/dhcp/nm-dhcp-client.c | 72 ++++++++++++---------------------- src/core/dhcp/nm-dhcp-client.h | 14 +------ 2 files changed, 26 insertions(+), 60 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 3c0c748725..eed1a89bb7 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -74,18 +74,18 @@ G_STATIC_ASSERT(!(((pid_t) -1) > 0)); /*****************************************************************************/ -NM_UTILS_LOOKUP_STR_DEFINE(nm_dhcp_state_to_string, - NMDhcpState, - NM_UTILS_LOOKUP_DEFAULT(NULL), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_BOUND, "bound"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_DONE, "done"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_EXPIRE, "expire"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_EXTENDED, "extended"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_FAIL, "fail"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_NOOP, "noop"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_TERMINATED, "terminated"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_TIMEOUT, "timeout"), - NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_STATE_UNKNOWN, "unknown"), ); +NM_UTILS_LOOKUP_STR_DEFINE(nm_dhcp_client_event_type_to_string, + NMDhcpClientEventType, + NM_UTILS_LOOKUP_DEFAULT_NM_ASSERT(NULL), + NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_CLIENT_EVENT_TYPE_BOUND, "bound"), + NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE, "expire"), + NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED, "extended"), + NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_CLIENT_EVENT_TYPE_FAIL, "fail"), + NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED, + "terminated"), + NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT, "timeout"), + NM_UTILS_LOOKUP_STR_ITEM(NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED, + "unspecified"), ); /*****************************************************************************/ @@ -252,6 +252,7 @@ _nm_dhcp_client_notify(NMDhcpClient *self, GHashTable *options; const int IS_IPv4 = NM_IS_IPv4(priv->config.addr_family); nm_auto_unref_l3cd const NML3ConfigData *l3cd_merged = NULL; + char sbuf1[NM_HASH_OBFUSCATE_PTR_STR_BUF_SIZE]; nm_assert(NM_IN_SET(client_event_type, NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED, @@ -275,6 +276,10 @@ _nm_dhcp_client_notify(NMDhcpClient *self, nm_assert(!l3cd || NM_IS_L3_CONFIG_DATA(l3cd)); nm_assert(!l3cd || nm_l3_config_data_get_dhcp_lease(l3cd, priv->config.addr_family)); + _LOGT("notify: event=%s%s%s", + nm_dhcp_client_event_type_to_string(client_event_type), + NM_PRINT_FMT_QUOTED2(l3cd, "", ", l3cd=", NM_HASH_OBFUSCATE_PTR_STR(l3cd, sbuf1))); + if (l3cd) nm_l3_config_data_seal(l3cd); @@ -890,7 +895,6 @@ 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, }; @@ -976,50 +980,24 @@ nm_dhcp_client_handle_event(gpointer unused, /* Fail if no valid IP config was received */ if (reason_is_bound && !l3cd) { _LOGW("client bound but IP config not received"); - new_state = NM_DHCP_STATE_FAIL; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; } else { if (NM_IN_STRSET_ASCII_CASE(reason, "bound", "bound6", "static")) - new_state = NM_DHCP_STATE_BOUND; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_BOUND; else if (NM_IN_STRSET_ASCII_CASE(reason, "renew", "renew6", "reboot", "rebind", "rebind6")) - new_state = NM_DHCP_STATE_EXTENDED; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED; else if (NM_IN_STRSET_ASCII_CASE(reason, "timeout")) - new_state = NM_DHCP_STATE_TIMEOUT; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT; else if (NM_IN_STRSET_ASCII_CASE(reason, "nak", "expire", "expire6")) - new_state = NM_DHCP_STATE_EXPIRE; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE; else if (NM_IN_STRSET_ASCII_CASE(reason, "end", "stop", "stopped")) - new_state = NM_DHCP_STATE_DONE; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED; else if (NM_IN_STRSET_ASCII_CASE(reason, "fail", "abend")) - new_state = NM_DHCP_STATE_FAIL; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; else if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) return TRUE; else - new_state = NM_DHCP_STATE_UNKNOWN; - } - - 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; + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED; } _nm_dhcp_client_notify(self, client_event_type, l3cd); diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index 95b2868759..7b88ae6b46 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -26,18 +26,6 @@ #define NM_DHCP_CLIENT_NOTIFY "dhcp-notify" -typedef enum { - NM_DHCP_STATE_UNKNOWN = 0, - NM_DHCP_STATE_NOOP, /* state is a non operation for NetworkManager */ - NM_DHCP_STATE_BOUND, /* new lease */ - NM_DHCP_STATE_EXTENDED, /* lease extended */ - NM_DHCP_STATE_TIMEOUT, /* timed out contacting server */ - NM_DHCP_STATE_DONE, /* client reported it's stopping */ - NM_DHCP_STATE_EXPIRE, /* lease expired or NAKed */ - NM_DHCP_STATE_FAIL, /* failed for some reason */ - NM_DHCP_STATE_TERMINATED, /* client is no longer running */ -} NMDhcpState; - typedef enum { NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED, @@ -94,7 +82,7 @@ typedef struct { }; } NMDhcpClientNotifyData; -const char *nm_dhcp_state_to_string(NMDhcpState state); +const char *nm_dhcp_client_event_type_to_string(NMDhcpClientEventType client_event_type); /* FIXME(l3cfg:dhcp:config): nm_dhcp_manager_start_ip[46]() has a gazillion of parameters, * those get passed on as CONSTRUCT_ONLY properties to the NMDhcpClient. Drop From 9097679aadc439c23a3f45b47a7eb8b5ca1a7712 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 21:09:20 +0200 Subject: [PATCH 18/24] dhcp: move code in nm_dhcp_client_handle_event() --- src/core/dhcp/nm-dhcp-client.c | 52 ++++++++++++++-------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index eed1a89bb7..ccb1cc5279 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -898,7 +898,6 @@ nm_dhcp_client_handle_event(gpointer unused, NMPlatformIP6Address prefix = { 0, }; - gboolean reason_is_bound; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); g_return_val_if_fail(iface != NULL, FALSE); @@ -918,17 +917,24 @@ nm_dhcp_client_handle_event(gpointer unused, if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) return TRUE; - reason_is_bound = NM_IN_STRSET_ASCII_CASE(reason, - "bound", - "bound6", - "static", - "renew", - "renew6", - "reboot", - "rebind", - "rebind6"); + if (NM_IN_STRSET_ASCII_CASE(reason, "bound", "bound6", "static")) + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_BOUND; + else if (NM_IN_STRSET_ASCII_CASE(reason, "renew", "renew6", "reboot", "rebind", "rebind6")) + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED; + else if (NM_IN_STRSET_ASCII_CASE(reason, "timeout")) + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT; + else if (NM_IN_STRSET_ASCII_CASE(reason, "nak", "expire", "expire6")) + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE; + else if (NM_IN_STRSET_ASCII_CASE(reason, "end", "stop", "stopped")) + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED; + else if (NM_IN_STRSET_ASCII_CASE(reason, "fail", "abend")) + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; + else + client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED; - if (reason_is_bound) { + if (NM_IN_SET(client_event_type, + NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED)) { gs_unref_hashtable GHashTable *str_options = NULL; GVariantIter iter; const char *name; @@ -977,27 +983,13 @@ nm_dhcp_client_handle_event(gpointer unused, return TRUE; } - /* Fail if no valid IP config was received */ - if (reason_is_bound && !l3cd) { + if (NM_IN_SET(client_event_type, + NM_DHCP_CLIENT_EVENT_TYPE_BOUND, + NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED) + && !l3cd) { + /* Fail if no valid IP config was received */ _LOGW("client bound but IP config not received"); client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; - } else { - if (NM_IN_STRSET_ASCII_CASE(reason, "bound", "bound6", "static")) - client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_BOUND; - else if (NM_IN_STRSET_ASCII_CASE(reason, "renew", "renew6", "reboot", "rebind", "rebind6")) - client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXTENDED; - else if (NM_IN_STRSET_ASCII_CASE(reason, "timeout")) - client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TIMEOUT; - else if (NM_IN_STRSET_ASCII_CASE(reason, "nak", "expire", "expire6")) - client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_EXPIRE; - else if (NM_IN_STRSET_ASCII_CASE(reason, "end", "stop", "stopped")) - client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_TERMINATED; - else if (NM_IN_STRSET_ASCII_CASE(reason, "fail", "abend")) - client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_FAIL; - else if (NM_IN_STRSET_ASCII_CASE(reason, "preinit")) - return TRUE; - else - client_event_type = NM_DHCP_CLIENT_EVENT_TYPE_UNSPECIFIED; } _nm_dhcp_client_notify(self, client_event_type, l3cd); From 892cde1436ce18c1e767aecd668f2a0621a57e45 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 21:10:34 +0200 Subject: [PATCH 19/24] dhcp: remove assertion in nm_dhcp_client_handle_event() Technically, g_warn_if_reached() may not be an assertion, according to glib. However, there is G_DEBUG=fatal-warnings and we want to run with that. So this is an assertion to us. Also, logging to stderr/stdout is not a useful thing to the daemon. Don't do this. Especially, since it depends on user provided (untrusted) input. --- src/core/dhcp/nm-dhcp-client.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index ccb1cc5279..a98dee13dd 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -965,8 +965,7 @@ nm_dhcp_client_handle_event(gpointer unused, str_options, priv->config.v6.info_only); } - } else - g_warn_if_reached(); + } if (l3cd) { nm_l3_config_data_set_dhcp_lease_from_options(l3cd, From f0ec2977397f4102d919cfde4ae1751cd17c313e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 21:15:46 +0200 Subject: [PATCH 20/24] dhcp: use packed strv array for NMDhcpClientConfig.reject_servers No need to do it otherwise. --- src/core/dhcp/nm-dhcp-client.c | 35 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index a98dee13dd..b61928ede5 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -1005,23 +1005,23 @@ nm_dhcp_client_server_id_is_rejected(NMDhcpClient *self, gconstpointer addr) /* IPv6 not implemented yet */ nm_assert(priv->config.addr_family == AF_INET); - if (!priv->config.reject_servers || !priv->config.reject_servers[0]) - return FALSE; + if (priv->config.reject_servers) { + for (i = 0; priv->config.reject_servers[i]; i++) { + in_addr_t r_addr; + in_addr_t mask; + int r_prefix; - for (i = 0; priv->config.reject_servers[i]; i++) { - in_addr_t r_addr; - in_addr_t mask; - int r_prefix; + if (!nm_utils_parse_inaddr_prefix_bin(AF_INET, + priv->config.reject_servers[i], + NULL, + &r_addr, + &r_prefix)) + nm_assert_not_reached(); - if (!nm_utils_parse_inaddr_prefix_bin(AF_INET, - priv->config.reject_servers[i], - NULL, - &r_addr, - &r_prefix)) - nm_assert_not_reached(); - mask = _nm_utils_ip4_prefix_to_netmask(r_prefix < 0 ? 32 : r_prefix); - if ((addr4 & mask) == (r_addr & mask)) - return TRUE; + mask = _nm_utils_ip4_prefix_to_netmask(r_prefix < 0 ? 32 : r_prefix); + if ((addr4 & mask) == (r_addr & mask)) + return TRUE; + } } return FALSE; @@ -1049,7 +1049,7 @@ config_init(NMDhcpClientConfig *config, const NMDhcpClientConfig *src) config->hostname = g_strdup(config->hostname); config->mud_url = g_strdup(config->mud_url); - config->reject_servers = (const char *const *) nm_strv_dup(config->reject_servers, -1, TRUE); + config->reject_servers = nm_strv_dup_packed(config->reject_servers, -1); if (config->addr_family == AF_INET) { config->v4.last_address = g_strdup(config->v4.last_address); @@ -1110,8 +1110,7 @@ config_clear(NMDhcpClientConfig *config) nm_clear_g_free((gpointer *) &config->anycast_address); nm_clear_g_free((gpointer *) &config->hostname); nm_clear_g_free((gpointer *) &config->mud_url); - - nm_clear_pointer((gpointer *) &config->reject_servers, g_strfreev); + nm_clear_g_free((gpointer *) &config->reject_servers); if (config->addr_family == AF_INET) { nm_clear_g_free((gpointer *) &config->v4.last_address); From 600467b96fd6c459839dcea44e5f5ac11a70b3e6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 21:18:50 +0200 Subject: [PATCH 21/24] dhcp: minor cleanup in config_init() --- src/core/dhcp/nm-dhcp-client.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index b61928ede5..d793aaed7a 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -1027,21 +1027,25 @@ nm_dhcp_client_server_id_is_rejected(NMDhcpClient *self, gconstpointer addr) return FALSE; } +/*****************************************************************************/ + static void config_init(NMDhcpClientConfig *config, const NMDhcpClientConfig *src) { + nm_assert(config); + nm_assert(src); + nm_assert(config != src); + *config = *src; + /* We must not return before un-aliasing all pointers in @config! */ + g_object_ref(config->l3cfg); - if (config->hwaddr) - g_bytes_ref(config->hwaddr); - if (config->bcast_hwaddr) - g_bytes_ref(config->bcast_hwaddr); - if (config->vendor_class_identifier) - g_bytes_ref(config->vendor_class_identifier); - if (config->client_id) - g_bytes_ref(config->client_id); + nm_g_bytes_ref(config->hwaddr); + nm_g_bytes_ref(config->bcast_hwaddr); + nm_g_bytes_ref(config->vendor_class_identifier); + nm_g_bytes_ref(config->client_id); config->iface = g_strdup(config->iface); config->uuid = g_strdup(config->uuid); @@ -1051,14 +1055,12 @@ config_init(NMDhcpClientConfig *config, const NMDhcpClientConfig *src) config->reject_servers = nm_strv_dup_packed(config->reject_servers, -1); - if (config->addr_family == AF_INET) { + if (NM_IS_IPv4(config->addr_family)) config->v4.last_address = g_strdup(config->v4.last_address); - } else if (config->addr_family == AF_INET6) { + else { config->hwaddr = NULL; config->bcast_hwaddr = NULL; config->use_fqdn = TRUE; - } else { - nm_assert_not_reached(); } if (!config->hostname && config->send_hostname) { @@ -1117,6 +1119,8 @@ config_clear(NMDhcpClientConfig *config) } } +/*****************************************************************************/ + int nm_dhcp_client_get_addr_family(NMDhcpClient *self) { From ea13cff76c9d5942f6ced41669297f77fea08a6a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 21:35:52 +0200 Subject: [PATCH 22/24] dhcp: assert that resources are freed in NMDhcpClient.dispose() --- src/core/dhcp/nm-dhcp-client.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index d793aaed7a..390bb594bf 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -1206,6 +1206,10 @@ dispose(GObject *object) nm_clear_g_source_inst(&priv->ipv6_lladdr_timeout_source); nm_clear_pointer(&priv->effective_client_id, g_bytes_unref); + nm_assert(!priv->watch_source); + nm_assert(!priv->l3cd); + nm_assert(priv->l3cfg_notify.id == 0); + G_OBJECT_CLASS(nm_dhcp_client_parent_class)->dispose(object); } From 2b8aeba06d6d2ac6602c0850652df1d3fb9eb495 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 21:38:49 +0200 Subject: [PATCH 23/24] dhcp: move code in "nm-dhcp-client.c" (2) --- src/core/dhcp/nm-dhcp-client.c | 170 +++++++++++++++++---------------- 1 file changed, 86 insertions(+), 84 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 390bb594bf..dfdfca940b 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -89,33 +89,52 @@ NM_UTILS_LOOKUP_STR_DEFINE(nm_dhcp_client_event_type_to_string, /*****************************************************************************/ -static void -_emit_notify(NMDhcpClient *self, const NMDhcpClientNotifyData *notify_data) -{ - g_signal_emit(G_OBJECT(self), signals[SIGNAL_NOTIFY], 0, notify_data); -} - -/*****************************************************************************/ - -static void -connect_l3cfg_notify(NMDhcpClient *self) +int +nm_dhcp_client_get_addr_family(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; + return priv->config.addr_family; +} - if (!do_connect) { - nm_clear_g_signal_handler(priv->config.l3cfg, &priv->l3cfg_notify.id); - return; - } +const char * +nm_dhcp_client_get_iface(NMDhcpClient *self) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - 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); - } + return priv->config.iface; +} + +NMDedupMultiIndex * +nm_dhcp_client_get_multi_idx(NMDhcpClient *self) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + return nm_l3cfg_get_multi_idx(priv->config.l3cfg); +} + +int +nm_dhcp_client_get_ifindex(NMDhcpClient *self) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + return nm_l3cfg_get_ifindex(priv->config.l3cfg); +} + +const NMDhcpClientConfig * +nm_dhcp_client_get_config(NMDhcpClient *self) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + return &priv->config; +} + +GBytes * +nm_dhcp_client_get_effective_client_id(NMDhcpClient *self) +{ + NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + + return priv->effective_client_id; } /*****************************************************************************/ @@ -144,12 +163,35 @@ nm_dhcp_client_set_effective_client_id(NMDhcpClient *self, GBytes *client_id) : "default"); } -const NMDhcpClientConfig * -nm_dhcp_client_get_config(NMDhcpClient *self) +/*****************************************************************************/ + +static void +_emit_notify(NMDhcpClient *self, const NMDhcpClientNotifyData *notify_data) +{ + g_signal_emit(G_OBJECT(self), signals[SIGNAL_NOTIFY], 0, notify_data); +} + +/*****************************************************************************/ + +static void +connect_l3cfg_notify(NMDhcpClient *self) { NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + gboolean do_connect; - return &priv->config; + 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); + } } /*****************************************************************************/ @@ -424,23 +466,6 @@ nm_dhcp_client_stop_watch_child(NMDhcpClient *self, pid_t pid) watch_cleanup(self); } -gboolean -nm_dhcp_client_start_ip4(NMDhcpClient *self, GError **error) -{ - NMDhcpClientPrivate *priv; - - g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); - - priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - g_return_val_if_fail(priv->pid == -1, FALSE); - g_return_val_if_fail(priv->config.addr_family == AF_INET, FALSE); - g_return_val_if_fail(priv->config.uuid, FALSE); - - _no_lease_timeout_schedule(self); - - return NM_DHCP_CLIENT_GET_CLASS(self)->ip4_start(self, error); -} - gboolean nm_dhcp_client_accept(NMDhcpClient *self, GError **error) { @@ -643,6 +668,23 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp } } +gboolean +nm_dhcp_client_start_ip4(NMDhcpClient *self, GError **error) +{ + NMDhcpClientPrivate *priv; + + g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); + + priv = NM_DHCP_CLIENT_GET_PRIVATE(self); + g_return_val_if_fail(priv->pid == -1, FALSE); + g_return_val_if_fail(priv->config.addr_family == AF_INET, FALSE); + g_return_val_if_fail(priv->config.uuid, FALSE); + + _no_lease_timeout_schedule(self); + + return NM_DHCP_CLIENT_GET_CLASS(self)->ip4_start(self, error); +} + gboolean nm_dhcp_client_start_ip6(NMDhcpClient *self, GError **error) { @@ -678,6 +720,8 @@ nm_dhcp_client_start_ip6(NMDhcpClient *self, GError **error) return NM_DHCP_CLIENT_GET_CLASS(self)->ip6_start(self, &addr->address, error); } +/*****************************************************************************/ + void nm_dhcp_client_stop_existing(const char *pid_file, const char *binary_name) { @@ -1121,48 +1165,6 @@ config_clear(NMDhcpClientConfig *config) /*****************************************************************************/ -int -nm_dhcp_client_get_addr_family(NMDhcpClient *self) -{ - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - - return priv->config.addr_family; -} - -const char * -nm_dhcp_client_get_iface(NMDhcpClient *self) -{ - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - - return priv->config.iface; -} - -NMDedupMultiIndex * -nm_dhcp_client_get_multi_idx(NMDhcpClient *self) -{ - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - - return nm_l3cfg_get_multi_idx(priv->config.l3cfg); -} - -int -nm_dhcp_client_get_ifindex(NMDhcpClient *self) -{ - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - - return nm_l3cfg_get_ifindex(priv->config.l3cfg); -} - -GBytes * -nm_dhcp_client_get_effective_client_id(NMDhcpClient *self) -{ - NMDhcpClientPrivate *priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - - return priv->effective_client_id; -} - -/*****************************************************************************/ - static void set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec) { From 7f943f5fa6bbc799cde40f745496fd69ff5b0e38 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 May 2022 22:04:08 +0200 Subject: [PATCH 24/24] dhcp: merge nm_dhcp_client_start_ip4() and nm_dhcp_client_start_ip6() implementations As almost always, there is a point in keeping IPv4 and IPv6 implementations similar. Behave different where there is an actual difference, at the bottom of the stack. --- src/core/dhcp/nm-dhcp-client.c | 55 ++++++++++++++------------------- src/core/dhcp/nm-dhcp-client.h | 3 +- src/core/dhcp/nm-dhcp-manager.c | 11 ++----- 3 files changed, 26 insertions(+), 43 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index dfdfca940b..b1e1f9ab3a 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -669,54 +669,45 @@ l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, NMDhcp } gboolean -nm_dhcp_client_start_ip4(NMDhcpClient *self, GError **error) -{ - NMDhcpClientPrivate *priv; - - g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); - - priv = NM_DHCP_CLIENT_GET_PRIVATE(self); - g_return_val_if_fail(priv->pid == -1, FALSE); - g_return_val_if_fail(priv->config.addr_family == AF_INET, FALSE); - g_return_val_if_fail(priv->config.uuid, FALSE); - - _no_lease_timeout_schedule(self); - - return NM_DHCP_CLIENT_GET_CLASS(self)->ip4_start(self, error); -} - -gboolean -nm_dhcp_client_start_ip6(NMDhcpClient *self, GError **error) +nm_dhcp_client_start(NMDhcpClient *self, GError **error) { NMDhcpClientPrivate *priv; gs_unref_bytes GBytes *own_client_id = NULL; - const NMPlatformIP6Address *addr; + const NMPlatformIP6Address *addr = NULL; + int IS_IPv4; g_return_val_if_fail(NM_IS_DHCP_CLIENT(self), FALSE); + priv = NM_DHCP_CLIENT_GET_PRIVATE(self); g_return_val_if_fail(priv->pid == -1, FALSE); - g_return_val_if_fail(priv->config.addr_family == AF_INET6, FALSE); g_return_val_if_fail(priv->config.uuid, FALSE); - g_return_val_if_fail(!priv->effective_client_id, FALSE); + nm_assert(!priv->effective_client_id); - if (!priv->config.v6.enforce_duid) - own_client_id = NM_DHCP_CLIENT_GET_CLASS(self)->get_duid(self); + IS_IPv4 = NM_IS_IPv4(priv->config.addr_family); - nm_dhcp_client_set_effective_client_id(self, own_client_id ?: priv->config.client_id); + if (!IS_IPv4) { + if (!priv->config.v6.enforce_duid) + own_client_id = NM_DHCP_CLIENT_GET_CLASS(self)->get_duid(self); - addr = ipv6_lladdr_find(self); - if (!addr) { - _LOGD("waiting for IPv6LL address"); - 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; + nm_dhcp_client_set_effective_client_id(self, own_client_id ?: priv->config.client_id); + + addr = ipv6_lladdr_find(self); + if (!addr) { + _LOGD("waiting for IPv6LL address"); + 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; + } } _no_lease_timeout_schedule(self); + if (IS_IPv4) + return NM_DHCP_CLIENT_GET_CLASS(self)->ip4_start(self, error); + return NM_DHCP_CLIENT_GET_CLASS(self)->ip6_start(self, &addr->address, error); } diff --git a/src/core/dhcp/nm-dhcp-client.h b/src/core/dhcp/nm-dhcp-client.h index 7b88ae6b46..274a954b6c 100644 --- a/src/core/dhcp/nm-dhcp-client.h +++ b/src/core/dhcp/nm-dhcp-client.h @@ -230,8 +230,7 @@ typedef struct { GType nm_dhcp_client_get_type(void); -gboolean nm_dhcp_client_start_ip4(NMDhcpClient *self, GError **error); -gboolean nm_dhcp_client_start_ip6(NMDhcpClient *self, GError **error); +gboolean nm_dhcp_client_start(NMDhcpClient *self, GError **error); const NMDhcpClientConfig *nm_dhcp_client_get_config(NMDhcpClient *self); diff --git a/src/core/dhcp/nm-dhcp-manager.c b/src/core/dhcp/nm-dhcp-manager.c index f353e63773..9fea166614 100644 --- a/src/core/dhcp/nm-dhcp-manager.c +++ b/src/core/dhcp/nm-dhcp-manager.c @@ -131,8 +131,7 @@ NMDhcpClient * nm_dhcp_manager_start_client(NMDhcpManager *self, NMDhcpClientConfig *config, GError **error) { NMDhcpManagerPrivate *priv; - gs_unref_object NMDhcpClient *client = NULL; - gboolean success = FALSE; + gs_unref_object NMDhcpClient *client = NULL; gsize hwaddr_len; GType gtype; @@ -202,13 +201,7 @@ nm_dhcp_manager_start_client(NMDhcpManager *self, NMDhcpClientConfig *config, GE * default outside of NetworkManager API. */ - if (config->addr_family == AF_INET) { - success = nm_dhcp_client_start_ip4(client, error); - } else { - success = nm_dhcp_client_start_ip6(client, error); - } - - if (!success) + if (!nm_dhcp_client_start(client, error)) return NULL; return g_steal_pointer(&client);