From 405a2fa166f77d273b87dae8c855a1360764cfb2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 23 Nov 2023 16:44:38 +0100 Subject: [PATCH 1/7] libnm/tests: add more tests about dns-options in NMSettingIPConfig --- src/libnm-core-impl/tests/test-general.c | 60 ++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index c8952bf3ed..d4ee8616eb 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -5311,6 +5311,7 @@ test_setting_ip4_changed_signal(void) NMIPAddress *addr; NMIPRoute *route; GError *error = NULL; + gs_strfreev char **strv = NULL; connection = nm_simple_connection_new(); g_signal_connect(connection, @@ -5368,9 +5369,55 @@ test_setting_ip4_changed_signal(void) nm_setting_ip_config_add_route(s_ip4, route); ASSERT_CHANGED(nm_setting_ip_config_clear_routes(s_ip4)); + g_assert(!nm_setting_ip_config_has_dns_options(s_ip4)); + g_assert_cmpint(nm_setting_ip_config_get_num_dns_options(s_ip4), ==, 0); + + g_object_get(s_ip4, NM_SETTING_IP_CONFIG_DNS_OPTIONS, &strv, NULL); + g_assert_null(strv); + + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(priv->dns_options)); + g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 0)); + g_test_assert_expected_messages(); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(priv->dns_options)); + g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 1)); + g_test_assert_expected_messages(); + ASSERT_CHANGED(nm_setting_ip_config_add_dns_option(s_ip4, "debug")); + + g_assert(nm_setting_ip_config_has_dns_options(s_ip4)); + g_assert_cmpint(nm_setting_ip_config_get_num_dns_options(s_ip4), ==, 1); + + g_object_get(s_ip4, NM_SETTING_IP_CONFIG_DNS_OPTIONS, &strv, NULL); + g_assert_nonnull(strv); + g_assert_cmpstr(strv[0], ==, "debug"); + g_assert_cmpstr(strv[1], ==, NULL); + nm_clear_pointer(&strv, g_strfreev); + + g_assert_cmpstr(nm_setting_ip_config_get_dns_option(s_ip4, 0), ==, "debug"); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); + g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 1)); + g_test_assert_expected_messages(); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); + g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 2)); + g_test_assert_expected_messages(); + ASSERT_CHANGED(nm_setting_ip_config_remove_dns_option(s_ip4, 0)); + g_assert(nm_setting_ip_config_has_dns_options(s_ip4)); + g_assert_cmpint(nm_setting_ip_config_get_num_dns_options(s_ip4), ==, 0); + + g_object_get(s_ip4, NM_SETTING_IP_CONFIG_DNS_OPTIONS, &strv, NULL); + g_assert_nonnull(strv); + g_assert_cmpstr(strv[0], ==, NULL); + nm_clear_pointer(&strv, g_strfreev); + + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); + g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 0)); + g_test_assert_expected_messages(); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); + g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 1)); + g_test_assert_expected_messages(); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx >= 0 && idx < priv->dns_options->len)); ASSERT_UNCHANGED(nm_setting_ip_config_remove_dns_option(s_ip4, 1)); g_test_assert_expected_messages(); @@ -5383,6 +5430,7 @@ test_setting_ip4_changed_signal(void) static void test_setting_ip6_changed_signal(void) { + gs_strfreev char **strv = NULL; NMConnection *connection; gboolean changed = FALSE; NMSettingIPConfig *s_ip6; @@ -5410,8 +5458,17 @@ test_setting_ip6_changed_signal(void) nm_setting_ip_config_add_dns(s_ip6, "1:2:3::4:5:6"); ASSERT_CHANGED(nm_setting_ip_config_clear_dns(s_ip6)); + g_object_get(s_ip6, NM_SETTING_IP_CONFIG_DNS_SEARCH, &strv, NULL); + g_assert_null(strv); + ASSERT_CHANGED(nm_setting_ip_config_add_dns_search(s_ip6, "foobar.com")); + g_object_get(s_ip6, NM_SETTING_IP_CONFIG_DNS_SEARCH, &strv, NULL); + g_assert_nonnull(strv); + g_assert_cmpstr(strv[0], ==, "foobar.com"); + g_assert_cmpstr(strv[1], ==, NULL); + nm_clear_pointer(&strv, g_strfreev); + g_assert_cmpstr(nm_setting_ip_config_get_dns_search(s_ip6, 0), ==, "foobar.com"); g_assert_cmpstr(nm_setting_ip_config_get_dns_search(s_ip6, 1), ==, NULL); @@ -5425,6 +5482,9 @@ test_setting_ip6_changed_signal(void) ASSERT_CHANGED(nm_setting_ip_config_remove_dns_search(s_ip6, 0)); + g_object_get(s_ip6, NM_SETTING_IP_CONFIG_DNS_SEARCH, &strv, NULL); + g_assert_null(strv); + NMTST_EXPECT_LIBNM_CRITICAL( NMTST_G_RETURN_MSG(idx >= 0 && idx < nm_g_array_len(priv->dns_search.arr))); ASSERT_UNCHANGED(nm_setting_ip_config_remove_dns_search(s_ip6, 1)); From 3f8431f0693967547d39c711328f72c9c9b3289b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 15 Nov 2023 19:53:44 +0100 Subject: [PATCH 2/7] libnm: refactor "ipv6" argument of _nm_utils_dns_option_validate() _nm_utils_dns_option_validate() allows specifying the address family, and filters based on that. Note that all options are valid for IPv6, but some are not valid for IPv4. It's not obvious, that such filtering is only performed if "option_descs" argument is provied. Otherwise, the "ipv6" argument is ignored. Regardless, it's also confusing to have a boolean "ipv6". When most callers don't want a filtering based on the address family. They actually don't want any filtering at all, as they don't pass an "option_descs". At the same time passing a TRUE/FALSE "ipv6" is redundant and ignored. It should be possible, to explicitly not select an address family (as it's ignored anyway). Instead, make the "gboolean ipv6" argument an "int addr_family". Selecting AF_UNSPEC means clearly to accept any address family. --- src/core/nm-config-data.c | 4 +- src/libnm-core-impl/nm-setting-ip-config.c | 6 +- src/libnm-core-impl/nm-utils.c | 29 ++++++--- src/libnm-core-impl/tests/test-general.c | 72 +++++++++++++--------- src/libnm-core-intern/nm-core-internal.h | 2 +- 5 files changed, 68 insertions(+), 45 deletions(-) diff --git a/src/core/nm-config-data.c b/src/core/nm-config-data.c index ed6d8381f2..468e56b821 100644 --- a/src/core/nm-config-data.c +++ b/src/core/nm-config-data.c @@ -1219,7 +1219,7 @@ load_global_dns(GKeyFile *keyfile, gboolean internal) if (strv) { nm_strv_cleanup(strv, TRUE, TRUE, TRUE); for (i = 0, j = 0; strv[i]; i++) { - if (_nm_utils_dns_option_validate(strv[i], NULL, NULL, TRUE, NULL)) + if (_nm_utils_dns_option_validate(strv[i], NULL, NULL, AF_UNSPEC, NULL)) strv[j++] = strv[i]; else g_free(strv[i]); @@ -1453,7 +1453,7 @@ nm_global_dns_config_from_dbus(const GValue *value, GError **error) nm_strv_cleanup(strv, TRUE, TRUE, TRUE); for (i = 0, j = 0; strv && strv[i]; i++) { - if (_nm_utils_dns_option_validate(strv[i], NULL, NULL, TRUE, NULL)) + if (_nm_utils_dns_option_validate(strv[i], NULL, NULL, AF_UNSPEC, NULL)) strv[j++] = strv[i]; else g_free(strv[i]); diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 06e250dcbb..720048c3bb 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -4434,7 +4434,7 @@ nm_setting_ip_config_next_valid_dns_option(NMSettingIPConfig *setting, guint idx if (_nm_utils_dns_option_validate(priv->dns_options->pdata[idx], NULL, NULL, - NM_IS_SETTING_IP6_CONFIG(setting), + NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY(setting), _nm_utils_dns_option_descs)) return idx; } @@ -4462,7 +4462,7 @@ nm_setting_ip_config_add_dns_option(NMSettingIPConfig *setting, const char *dns_ g_return_val_if_fail(dns_option != NULL, FALSE); g_return_val_if_fail(dns_option[0] != '\0', FALSE); - if (!_nm_utils_dns_option_validate(dns_option, NULL, NULL, FALSE, NULL)) + if (!_nm_utils_dns_option_validate(dns_option, NULL, NULL, AF_UNSPEC, NULL)) return FALSE; priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); @@ -6208,7 +6208,7 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps else priv->dns_options = g_ptr_array_new_with_free_func(g_free); for (i = 0; strv[i]; i++) { - if (_nm_utils_dns_option_validate(strv[i], NULL, NULL, FALSE, NULL) + if (_nm_utils_dns_option_validate(strv[i], NULL, NULL, AF_UNSPEC, NULL) && _nm_utils_dns_option_find_idx(priv->dns_options, strv[i]) < 0) g_ptr_array_add(priv->dns_options, g_strdup(strv[i])); } diff --git a/src/libnm-core-impl/nm-utils.c b/src/libnm-core-impl/nm-utils.c index c476d18661..e59e572ef3 100644 --- a/src/libnm-core-impl/nm-utils.c +++ b/src/libnm-core-impl/nm-utils.c @@ -4694,7 +4694,7 @@ _nm_utils_strstrdictkey_create(const char *v1, const char *v2) static gboolean validate_dns_option(const char *name, gboolean numeric, - gboolean ipv6, + int addr_family, const NMUtilsDNSOptionDesc *option_descs) { const NMUtilsDNSOptionDesc *desc; @@ -4703,8 +4703,15 @@ validate_dns_option(const char *name, return !!*name; for (desc = option_descs; desc->name; desc++) { - if (nm_streq(name, desc->name) && numeric == desc->numeric && (!desc->ipv6_only || ipv6)) - return TRUE; + if (!nm_streq(name, desc->name)) + continue; + if ((!!numeric) != (!!desc->numeric)) + continue; + if (addr_family != AF_UNSPEC) { + if (desc->ipv6_only && addr_family != AF_INET6) + continue; + } + return TRUE; } return FALSE; @@ -4715,7 +4722,9 @@ validate_dns_option(const char *name, * @option: option string * @out_name: (out) (optional) (nullable): the option name * @out_value: (out) (optional): the option value - * @ipv6: whether the option refers to a IPv6 configuration + * @addr_family: AF_INET/AF_INET6 to only allow options for the specified address + * family. AF_UNSPEC to allow either. This argument is ignored, if @option_descs + * is NULL. * @option_descs: (nullable): an array of NMUtilsDNSOptionDesc which describes the * valid options * @@ -4731,7 +4740,7 @@ gboolean _nm_utils_dns_option_validate(const char *option, char **out_name, long *out_value, - gboolean ipv6, + int addr_family, const NMUtilsDNSOptionDesc *option_descs) { gs_free char *option0_free = NULL; @@ -4742,6 +4751,8 @@ _nm_utils_dns_option_validate(const char *option, g_return_val_if_fail(option != NULL, FALSE); + nm_assert_addr_family_or_unspec(addr_family); + NM_SET_OUT(out_name, NULL); NM_SET_OUT(out_value, -1); @@ -4750,7 +4761,7 @@ _nm_utils_dns_option_validate(const char *option, delim = strchr(option, ':'); if (!delim) { - if (!validate_dns_option(option, FALSE, ipv6, option_descs)) + if (!validate_dns_option(option, FALSE, addr_family, option_descs)) return FALSE; NM_SET_OUT(out_name, g_strdup(option)); return TRUE; @@ -4765,7 +4776,7 @@ _nm_utils_dns_option_validate(const char *option, option0 = nm_strndup_a(300, option, delim - option, &option0_free); - if (!validate_dns_option(option0, TRUE, ipv6, option_descs)) + if (!validate_dns_option(option0, TRUE, addr_family, option_descs)) return FALSE; option1_num = _nm_utils_ascii_str_to_int64(option1, 10, 0, G_MAXINT32, -1); @@ -4794,13 +4805,13 @@ _nm_utils_dns_option_find_idx(GPtrArray *array, const char *option) gs_free char *option_name = NULL; guint i; - if (!_nm_utils_dns_option_validate(option, &option_name, NULL, FALSE, NULL)) + if (!_nm_utils_dns_option_validate(option, &option_name, NULL, AF_UNSPEC, NULL)) return -1; for (i = 0; i < array->len; i++) { gs_free char *tmp_name = NULL; - if (_nm_utils_dns_option_validate(array->pdata[i], &tmp_name, NULL, FALSE, NULL)) { + if (_nm_utils_dns_option_validate(array->pdata[i], &tmp_name, NULL, AF_UNSPEC, NULL)) { if (nm_streq(tmp_name, option_name)) return i; } diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index d4ee8616eb..a873861871 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -8803,23 +8803,35 @@ test_nm_ptrarray_len(void) static void test_nm_utils_dns_option_validate_do(char *option, - gboolean ipv6, + int addr_family, const NMUtilsDNSOptionDesc *descs, gboolean exp_result, char *exp_name, gboolean exp_value) { - char *name; - long value = 0; - gboolean result; + gs_free char *name = NULL; + long value = 0; + gboolean result; - result = _nm_utils_dns_option_validate(option, &name, &value, ipv6, descs); + if (!descs) { + g_assert(addr_family == AF_UNSPEC); + addr_family = nmtst_rand_select(AF_UNSPEC, AF_INET, AF_INET6); + } + + result = _nm_utils_dns_option_validate(option, &name, &value, addr_family, descs); g_assert(result == exp_result); g_assert_cmpstr(name, ==, exp_name); g_assert(value == exp_value); - g_free(name); + nm_clear_g_free(&name); + + if (result && descs) { + result = _nm_utils_dns_option_validate(option, &name, &value, AF_UNSPEC, descs); + g_assert(result == exp_result); + g_assert_cmpstr(name, ==, exp_name); + g_assert(value == exp_value); + } } static const NMUtilsDNSOptionDesc opt_descs[] = { @@ -8833,34 +8845,34 @@ static const NMUtilsDNSOptionDesc opt_descs[] = { static void test_nm_utils_dns_option_validate(void) { - /* opt ipv6 descs result name value */ - test_nm_utils_dns_option_validate_do("", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do(":", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do(":1", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do(":val", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt", FALSE, NULL, TRUE, "opt", -1); - test_nm_utils_dns_option_validate_do("opt:", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt:12", FALSE, NULL, TRUE, "opt", 12); - test_nm_utils_dns_option_validate_do("opt:12 ", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt:val", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt:2val", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt:2:3", FALSE, NULL, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt-6", FALSE, NULL, TRUE, "opt-6", -1); + /* (opt, addr_family, descs, result, name, value) */ + test_nm_utils_dns_option_validate_do("", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do(":", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do(":1", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do(":val", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt", AF_UNSPEC, NULL, TRUE, "opt", -1); + test_nm_utils_dns_option_validate_do("opt:", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt:12", AF_UNSPEC, NULL, TRUE, "opt", 12); + test_nm_utils_dns_option_validate_do("opt:12 ", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt:val", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt:2val", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt:2:3", AF_UNSPEC, NULL, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt-6", AF_UNSPEC, NULL, TRUE, "opt-6", -1); - test_nm_utils_dns_option_validate_do("opt1", FALSE, opt_descs, TRUE, "opt1", -1); - test_nm_utils_dns_option_validate_do("opt1", TRUE, opt_descs, TRUE, "opt1", -1); - test_nm_utils_dns_option_validate_do("opt1:3", FALSE, opt_descs, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt1", AF_INET, opt_descs, TRUE, "opt1", -1); + test_nm_utils_dns_option_validate_do("opt1", AF_INET6, opt_descs, TRUE, "opt1", -1); + test_nm_utils_dns_option_validate_do("opt1:3", AF_INET, opt_descs, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt2", FALSE, opt_descs, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt2:5", FALSE, opt_descs, TRUE, "opt2", 5); + test_nm_utils_dns_option_validate_do("opt2", AF_INET, opt_descs, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt2:5", AF_INET, opt_descs, TRUE, "opt2", 5); - test_nm_utils_dns_option_validate_do("opt3", FALSE, opt_descs, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt3", TRUE, opt_descs, TRUE, "opt3", -1); + test_nm_utils_dns_option_validate_do("opt3", AF_INET, opt_descs, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt3", AF_INET6, opt_descs, TRUE, "opt3", -1); - test_nm_utils_dns_option_validate_do("opt4", FALSE, opt_descs, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt4", TRUE, opt_descs, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt4:40", FALSE, opt_descs, FALSE, NULL, -1); - test_nm_utils_dns_option_validate_do("opt4:40", TRUE, opt_descs, TRUE, "opt4", 40); + test_nm_utils_dns_option_validate_do("opt4", AF_INET, opt_descs, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt4", AF_INET6, opt_descs, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt4:40", AF_INET, opt_descs, FALSE, NULL, -1); + test_nm_utils_dns_option_validate_do("opt4:40", AF_INET6, opt_descs, TRUE, "opt4", 40); } static void diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index 766c7dad1b..1679a724f2 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -411,7 +411,7 @@ extern const NMUtilsDNSOptionDesc _nm_utils_dns_option_descs[]; gboolean _nm_utils_dns_option_validate(const char *option, char **out_name, long *out_value, - gboolean ipv6, + int addr_family, const NMUtilsDNSOptionDesc *option_descs); gssize _nm_utils_dns_option_find_idx(GPtrArray *array, const char *option); From e48fc3ee3e1f9bfcf12ce4a44e0ec5919aa4ba42 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Nov 2023 12:07:33 +0100 Subject: [PATCH 3/7] glib-aux: add "const" to arguments of nm_strvarray_*() helpers --- src/libnm-glib-aux/nm-shared-utils.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 914a3283ea..25f11a9d03 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -3021,7 +3021,7 @@ nm_strvarray_add(GArray *array, const char *str) } static inline const char * -nm_strvarray_get_idx(GArray *array, guint idx) +nm_strvarray_get_idx(const GArray *array, guint idx) { return nm_g_array_index(array, const char *, idx); } @@ -3103,14 +3103,14 @@ nm_strvarray_set_strv(GArray **array, const char *const *strv) } static inline gssize -nm_strvarray_find_first(GArray *strv, const char *needle) +nm_strvarray_find_first(const GArray *strv, const char *needle) { guint i; nm_assert(needle); if (strv) { - nm_assert(sizeof(char *) == g_array_get_element_size(strv)); + nm_assert(sizeof(char *) == g_array_get_element_size((GArray *) strv)); for (i = 0; i < strv->len; i++) { if (nm_streq(needle, g_array_index(strv, const char *, i))) return i; From 563fad718c6f9f6642becf1fd5b0cda0841917bb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Nov 2023 10:34:11 +0100 Subject: [PATCH 4/7] glib-aux: refactor nm_strvarray_get_strv*() and nm_strvarray_set_strv*() helpers Unfortunately, there are several possibilities how to handle NULL and empty arrays. Therefore we have different variants. Clean this up, and add a way to preserve whether the array is empty (previous variants could not distinguish that). Functions are also renamed, so that if you backport a user of the new API, you'll get a compiler error if this patch is missing. Also, nm_strvarray_get_strv_notnull() no longer takes a pointer to a "GArray*". Previously, it used that to fake an empty strv array. Now this returns NM_STRV_EMPTY_CC(). --- src/libnm-core-impl/nm-connection.c | 4 +- src/libnm-core-impl/nm-setting-ip-config.c | 4 +- src/libnm-core-impl/nm-setting-match.c | 8 +- src/libnm-core-impl/nm-setting.c | 2 +- src/libnm-core-impl/tests/test-setting.c | 74 +++++---- src/libnm-glib-aux/nm-shared-utils.h | 179 ++++++++++++++++++--- 6 files changed, 198 insertions(+), 73 deletions(-) diff --git a/src/libnm-core-impl/nm-connection.c b/src/libnm-core-impl/nm-connection.c index 24304f7d62..d156915682 100644 --- a/src/libnm-core-impl/nm-connection.c +++ b/src/libnm-core-impl/nm-connection.c @@ -908,7 +908,7 @@ _nm_setting_connection_verify_secondaries(GArray *secondaries, GError **error) * Now, when we find any invalid/non-normalized values, we reject/normalize * them. We also filter out duplicates. */ - strv = nm_strvarray_get_strv_non_empty(secondaries, NULL); + strv = nm_strvarray_get_strv_notempty(secondaries, NULL); for (i = 0; i < len; i++) { const char *uuid = strv[i]; @@ -977,7 +977,7 @@ _normalize_connection_secondaries(NMConnection *self) if (_nm_setting_connection_verify_secondaries(secondaries, NULL)) return FALSE; - strv = nm_strvarray_get_strv_non_empty_dup(secondaries, NULL); + strv = nm_strvarray_get_strv_notempty_dup(secondaries, NULL); for (i = 0, j = 0; strv[i]; i++) { gs_free char *s = g_steal_pointer(&strv[i]); char uuid_normalized[37]; diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 720048c3bb..1cae1e4447 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -5352,8 +5352,8 @@ nm_setting_ip_config_get_dhcp_reject_servers(NMSettingIPConfig *setting, guint * { g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), NULL); - return nm_strvarray_get_strv( - &NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dhcp_reject_servers.arr, + return nm_strvarray_get_strv_notnull( + NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dhcp_reject_servers.arr, out_len); } diff --git a/src/libnm-core-impl/nm-setting-match.c b/src/libnm-core-impl/nm-setting-match.c index 4cad4f68a1..7736b7c0b1 100644 --- a/src/libnm-core-impl/nm-setting-match.c +++ b/src/libnm-core-impl/nm-setting-match.c @@ -182,7 +182,7 @@ nm_setting_match_get_interface_names(NMSettingMatch *setting, guint *length) { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), NULL); - return nm_strvarray_get_strv(&setting->interface_name.arr, length); + return nm_strvarray_get_strv_notnull(setting->interface_name.arr, length); } /*****************************************************************************/ @@ -320,7 +320,7 @@ nm_setting_match_get_kernel_command_lines(NMSettingMatch *setting, guint *length { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), NULL); - return nm_strvarray_get_strv(&setting->kernel_command_line.arr, length); + return nm_strvarray_get_strv_notnull(setting->kernel_command_line.arr, length); } /*****************************************************************************/ @@ -456,7 +456,7 @@ nm_setting_match_get_drivers(NMSettingMatch *setting, guint *length) { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), NULL); - return nm_strvarray_get_strv(&setting->driver.arr, length); + return nm_strvarray_get_strv_notnull(setting->driver.arr, length); } /*****************************************************************************/ @@ -592,7 +592,7 @@ nm_setting_match_get_paths(NMSettingMatch *setting, guint *length) { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), NULL); - return nm_strvarray_get_strv(&setting->path.arr, length); + return nm_strvarray_get_strv_notnull(setting->path.arr, length); } /*****************************************************************************/ diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index c402174afe..d14fc35856 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -817,7 +817,7 @@ _nm_setting_property_get_property_direct(GObject *object, { const NMValueStrv *p_val = _nm_setting_get_private_field(setting, sett_info, property_info); - g_value_take_boxed(value, nm_strvarray_get_strv_non_empty_dup(p_val->arr, NULL)); + g_value_take_boxed(value, nm_strvarray_get_strv_notempty_dup(p_val->arr, NULL)); return; } default: diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index 1a8910d8d3..53a7acd2fc 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -5108,44 +5108,42 @@ test_setting_connection_secondaries_verify(void) g_object_set(s_con, NM_SETTING_CONNECTION_SECONDARIES, arr->pdata, NULL); -#define _assert_secondaries(s_con, expected) \ - G_STMT_START \ - { \ - NMSettingConnection *const _s_con = (s_con); \ - const char *const *_expected = (expected); \ - GArray *_secondaries; \ - const guint _expected_len = NM_PTRARRAY_LEN(_expected); \ - gs_strfreev char **_sec_strv = NULL; \ - guint _i; \ - \ - g_assert(_expected); \ - \ - if (nmtst_get_rand_bool()) { \ - _secondaries = _nm_setting_connection_get_secondaries(_s_con); \ - g_assert_cmpint(_expected_len, ==, nm_g_array_len(_secondaries)); \ - g_assert((_expected_len == 0) == (!_secondaries)); \ - g_assert(nm_strv_equal(_expected, \ - _secondaries ? nm_strvarray_get_strv(&_secondaries, NULL) \ - : NM_PTRARRAY_EMPTY(const char *))); \ - } \ - \ - if (nmtst_get_rand_bool()) { \ - g_object_get(_s_con, NM_SETTING_CONNECTION_SECONDARIES, &_sec_strv, NULL); \ - g_assert_cmpint(_expected_len, ==, NM_PTRARRAY_LEN(_sec_strv)); \ - g_assert((_expected_len == 0) == (!_sec_strv)); \ - g_assert(nm_strv_equal(_expected, _sec_strv ?: NM_STRV_EMPTY())); \ - } \ - \ - g_assert_cmpint(nm_setting_connection_get_num_secondaries(_s_con), ==, _expected_len); \ - if (nmtst_get_rand_bool()) { \ - for (_i = 0; _i < _expected_len; _i++) { \ - g_assert_cmpstr(nm_setting_connection_get_secondary(_s_con, _i), \ - ==, \ - _expected[_i]); \ - } \ - g_assert_null(nm_setting_connection_get_secondary(_s_con, _expected_len)); \ - } \ - } \ +#define _assert_secondaries(s_con, expected) \ + G_STMT_START \ + { \ + NMSettingConnection *const _s_con = (s_con); \ + const char *const *_expected = (expected); \ + GArray *_secondaries; \ + const guint _expected_len = NM_PTRARRAY_LEN(_expected); \ + gs_strfreev char **_sec_strv = NULL; \ + guint _i; \ + \ + g_assert(_expected); \ + \ + if (nmtst_get_rand_bool()) { \ + _secondaries = _nm_setting_connection_get_secondaries(_s_con); \ + g_assert_cmpint(_expected_len, ==, nm_g_array_len(_secondaries)); \ + g_assert((_expected_len == 0) == (!_secondaries)); \ + g_assert(nm_strv_equal(_expected, nm_strvarray_get_strv_notnull(_secondaries, NULL))); \ + } \ + \ + if (nmtst_get_rand_bool()) { \ + g_object_get(_s_con, NM_SETTING_CONNECTION_SECONDARIES, &_sec_strv, NULL); \ + g_assert_cmpint(_expected_len, ==, NM_PTRARRAY_LEN(_sec_strv)); \ + g_assert((_expected_len == 0) == (!_sec_strv)); \ + g_assert(nm_strv_equal(_expected, _sec_strv ?: NM_STRV_EMPTY())); \ + } \ + \ + g_assert_cmpint(nm_setting_connection_get_num_secondaries(_s_con), ==, _expected_len); \ + if (nmtst_get_rand_bool()) { \ + for (_i = 0; _i < _expected_len; _i++) { \ + g_assert_cmpstr(nm_setting_connection_get_secondary(_s_con, _i), \ + ==, \ + _expected[_i]); \ + } \ + g_assert_null(nm_setting_connection_get_secondary(_s_con, _expected_len)); \ + } \ + } \ G_STMT_END _assert_secondaries(s_con, (const char *const *) arr->pdata); diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 25f11a9d03..6c553cc085 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -3040,53 +3040,159 @@ nm_strvarray_get_idx(const GArray *array, guint idx) _idx == _len ? NULL : nm_strvarray_get_idx(_arr, _idx); \ }) +/** + * nm_strvarray_get_strv_full: + * @arr: the strvarray. + * @length: (out) (nullable): optionally return the length of the result. + * @not_null: if true and @arr is NULL, return NM_STRV_EMPTY_CC() (otherwise NULL). + * @preserve_empty: if true and the array is empty, return an empty + * strv array. Otherwise, return NULL. + * + * If "arr" is NULL, this returns NULL, unless "not_null" is true (in which + * case the static NM_STRV_EMPTY_CC() is returned). + * If "arr" is empty, it depends on: + * - if "preserve_empty" or "not_null", then the resulting strv array is the empty "arr". + * - otherwise NULL is returned. + * Otherwise, returns the non-empty, non-deep-cloned strv array. + * + * Like nm_strvarray_get_strv_full_dup(), but the strings are not cloned. + * + * Returns: (transfer none): a strv list or NULL. + */ static inline const char *const * -nm_strvarray_get_strv_non_empty(GArray *arr, guint *length) +nm_strvarray_get_strv_full(const GArray *arr, + guint *length, + gboolean not_null, + gboolean preserve_empty) { - nm_assert(!arr || sizeof(char *) == g_array_get_element_size(arr)); - - if (!arr || arr->len == 0) { + if (!arr) { NM_SET_OUT(length, 0); - return NULL; + return not_null ? NM_STRV_EMPTY_CC() : NULL; } + nm_assert(sizeof(char *) == g_array_get_element_size((GArray *) arr)); + NM_SET_OUT(length, arr->len); + + if (arr->len == 0 && !(preserve_empty || not_null)) + return NULL; + return &g_array_index(arr, const char *, 0); } +/** + * nm_strvarray_get_strv_full_dup: + * @arr: the strvarray. + * @length: (out) (nullable): optionally return the length of the result. + * @not_null: if true, never return NULL but allocate an empty strv array. + * @preserve_empty: if true and the array is empty, return an empty + * strv array. Otherwise, return NULL. + * + * If "arr" is NULL, this returns NULL, unless "not_null" is true (in which case + * am empty strv array is allocated. + * If "arr" is empty, it depends on: + * - if "preserve_empty" || "not_null", then the resulting strv array is allocated (and empty). + * - otherwise, NULL is returned. + * Otherwise, return the non-empty, deep-cloned strv array. + * + * Like nm_strvarray_get_strv_full(), but the strings are cloned. + * + * Returns: (transfer full): a deep-cloned strv list or NULL. + */ static inline char ** -nm_strvarray_get_strv_non_empty_dup(GArray *arr, guint *length) +nm_strvarray_get_strv_full_dup(const GArray *arr, + guint *length, + gboolean not_null, + gboolean preserve_empty) { - const char *const *strv; - - nm_assert(!arr || sizeof(char *) == g_array_get_element_size(arr)); - - if (!arr || arr->len == 0) { + if (!arr) { NM_SET_OUT(length, 0); + return not_null ? g_new0(char *, 1) : NULL; + } + + nm_assert(sizeof(char *) == g_array_get_element_size((GArray *) arr)); + + NM_SET_OUT(length, arr->len); + + if (arr->len == 0) { + if (preserve_empty || not_null) + return g_new0(char *, 1); return NULL; } - NM_SET_OUT(length, arr->len); - strv = &g_array_index(arr, const char *, 0); - return nm_strv_dup(strv, arr->len, TRUE); + return nm_strv_dup(&g_array_index(arr, const char *, 0), arr->len, TRUE); } +/** + * nm_strvarray_get_strv_notnull: + * @arr: the strvarray. + * @length: (out) (nullable): optionally return the length of the result. + * + * This never returns NULL. If @arr is NULL, this returns NM_STRV_EMPTY_CC(). + * + * Like nm_strvarray_get_strv_notempty(), but never returns NULL. + * + * Returns: (transfer none): a pointer to the strv list in @arr or NM_STRV_EMPTY_CC(). + */ static inline const char *const * -nm_strvarray_get_strv(GArray **arr, guint *length) +nm_strvarray_get_strv_notnull(const GArray *arr, guint *length) { - if (!*arr) { - NM_SET_OUT(length, 0); - return (const char *const *) arr; - } - - nm_assert(sizeof(char *) == g_array_get_element_size(*arr)); - - NM_SET_OUT(length, (*arr)->len); - return &g_array_index(*arr, const char *, 0); + return nm_strvarray_get_strv_full(arr, length, TRUE, TRUE); } +/** + * nm_strvarray_get_strv_notempty: + * @arr: the strvarray. + * @length: (out) (nullable): optionally return the length of the result. + * + * This never returns an empty strv array. If @arr is NULL or empty, this + * returns NULL. + * + * Like nm_strvarray_get_strv_notempty_dup(), but does not clone strings. + * + * Returns: (transfer none): a pointer to the strv list in @arr or NULL. + */ +static inline const char *const * +nm_strvarray_get_strv_notempty(const GArray *arr, guint *length) +{ + return nm_strvarray_get_strv_full(arr, length, FALSE, FALSE); +} + +/** + * nm_strvarray_get_strv_notempty_dup: + * @arr: the strvarray. + * @length: (out) (nullable): optionally return the length of the result. + * + * This never returns an empty strv array. If @arr is NULL or empty, this + * returns NULL. + * + * Like nm_strvarray_get_strv_notempty(), but clones strings. + * + * Returns: (transfer full): a deep-cloned strv list or NULL. + */ +static inline char ** +nm_strvarray_get_strv_notempty_dup(const GArray *arr, guint *length) +{ + return nm_strvarray_get_strv_full_dup(arr, length, FALSE, FALSE); +} + +/** + * nm_strvarray_set_strv_full: + * @array: a pointer to the array to set. + * @strv: the strv array. May be NULL. + * @preserve_empty: how to treat if strv is empty (strv[0]==NULL). + * + * The old array will be freed (in a way so that the function is self-assignment + * safe). + * + * If "strv" is NULL, then the resulting GArray is NULL. + * If "strv" is empty, then it depends on "preserve_empty": + * - if "preserve_empty", then the resulting GArray is allocated (and empty). + * - if "!preserve_empty", then the resulting GArray is NULL. + * If "strv" is not empty, a GArray gets allocated and the strv array deep-cloned. + */ static inline void -nm_strvarray_set_strv(GArray **array, const char *const *strv) +nm_strvarray_set_strv_full(GArray **array, const char *const *strv, gboolean preserve_empty) { gs_unref_array GArray *array_old = NULL; @@ -3094,14 +3200,35 @@ nm_strvarray_set_strv(GArray **array, const char *const *strv) nm_assert(!array_old || sizeof(char *) == g_array_get_element_size(array_old)); - if (!strv || !strv[0]) + if (!strv) return; + if (!strv[0] && !preserve_empty) { + /* An empty strv array is treated like NULL. Don't allocate a GArray. */ + return; + } + nm_strvarray_ensure(array); for (; strv[0]; strv++) nm_strvarray_add(*array, strv[0]); } +/** + * nm_strvarray_set_strv: + * @array: a pointer to the array to set. + * @strv: the strv array. May be NULL. + * + * The old array will be freed (in a way so that the function is self-assignment + * safe). + * + * Note that this will never initialize an empty GArray. If strv is NULL or + * empty, the @array pointer will be set to NULL. */ +static inline void +nm_strvarray_set_strv(GArray **array, const char *const *strv) +{ + nm_strvarray_set_strv_full(array, strv, FALSE); +} + static inline gssize nm_strvarray_find_first(const GArray *strv, const char *needle) { From 189bddc99b07ab23bd47e82790d6f9d0ba5f0011 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Nov 2023 12:41:02 +0100 Subject: [PATCH 5/7] libnm: handle empty strv array same as NULL for compare/to-dbus For now, our "direct" strv properties cannot distinguish between NULL/unset/default and empty. Adjust the to-dbus() and compare() hooks to honor that. --- src/libnm-core-impl/nm-setting.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index d14fc35856..35139ebadd 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -1281,6 +1281,11 @@ _nm_setting_property_to_dbus_fcn_direct(_NM_SETT_INFO_PROP_TO_DBUS_FCN_ARGS _nm_ (const NMValueStrv *) _nm_setting_get_private_field(setting, sett_info, property_info); if (!val->arr) return NULL; + if (val->arr->len == 0) { + /* This property does not treat empty strv arrays special. No need + * to export the value on D-Bus. */ + return NULL; + } return g_variant_new_strv(nm_g_array_data(val->arr), val->arr->len); } default: @@ -2579,8 +2584,19 @@ _nm_setting_property_compare_fcn_direct(_NM_SETT_INFO_PROP_COMPARE_FCN_ARGS _nm_ case NM_VALUE_TYPE_BYTES: return nm_g_bytes_equal0(*((const GBytes *const *) p_a), *((const GBytes *const *) p_b)); case NM_VALUE_TYPE_STRV: - return nm_strvarray_equal(((const NMValueStrv *) p_a)->arr, - ((const NMValueStrv *) p_b)->arr); + { + const NMValueStrv *v_a = p_a; + const NMValueStrv *v_b = p_b; + const GArray *a = v_a->arr; + const GArray *b = v_b->arr; + + /* NULL and empty are treated identical. Coerce to NULL. */ + if (a && a->len == 0) + a = NULL; + if (b && b->len == 0) + b = NULL; + return nm_strvarray_equal(a, b); + } default: return nm_assert_unreachable_val(TRUE); } From 9f4cd6b03f5fc3628328de4ea2b530b48c171a5d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Nov 2023 12:18:47 +0100 Subject: [PATCH 6/7] libnm: add option for direct STRV properties to preseve/distinguish empty arrays For most strv or string properties, we cannot distinguish between NULL/unset/default and empty. It would be difficult to enter in nmcli or grasp how it differs. There are probably many bugs, where we accept empty strings, and fail to handle them correctly. Anyway. For most strv arrays, and empty array and NULL/unset/default are treated the same. That means, g_object_get() tends to always return NULL (never an empty strv array) and g_object_set() of an empty strv array will internally leave the GArray at NULL. For a few properties, there is a difference. See "ipv[46].dns-options". See also "clear_emptyunset_fcn" hook in libnm-setting. Add a way to handle such strv properties with the "direct" mechanism. --- src/libnm-core-impl/nm-setting.c | 57 +++++++++++++++--------- src/libnm-core-impl/tests/test-setting.c | 2 + src/libnm-core-intern/nm-core-internal.h | 8 ++++ 3 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index 35139ebadd..f3399d58c8 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -729,6 +729,24 @@ out_take: return nm_strdup_reset_take(dst, s); } +static gboolean +_property_direct_set_strv(const NMSettInfoSetting *sett_info, + const NMSettInfoProperty *property_info, + NMSetting *setting, + const char *const *strv) +{ + NMValueStrv *p_val = _nm_setting_get_private_field(setting, sett_info, property_info); + + if (!property_info->direct_strv_preserve_empty && strv && !strv[0]) + strv = NULL; + + if (nm_strvarray_equal_strv(p_val->arr, strv, -1)) + return FALSE; + + nm_strvarray_set_strv_full(&p_val->arr, strv, property_info->direct_strv_preserve_empty); + return TRUE; +} + void _nm_setting_property_get_property_direct(GObject *object, guint prop_id, @@ -817,7 +835,12 @@ _nm_setting_property_get_property_direct(GObject *object, { const NMValueStrv *p_val = _nm_setting_get_private_field(setting, sett_info, property_info); - g_value_take_boxed(value, nm_strvarray_get_strv_notempty_dup(p_val->arr, NULL)); + g_value_take_boxed( + value, + nm_strvarray_get_strv_full_dup(p_val->arr, + NULL, + FALSE, + property_info->direct_strv_preserve_empty)); return; } default: @@ -956,17 +979,9 @@ _nm_setting_property_set_property_direct(GObject *object, goto out_notify; } case NM_VALUE_TYPE_STRV: - { - NMValueStrv *p_val = _nm_setting_get_private_field(setting, sett_info, property_info); - const char *const *v; - - v = g_value_get_boxed(value); - if (nm_strvarray_equal_strv(p_val->arr, v, -1)) + if (!_property_direct_set_strv(sett_info, property_info, setting, g_value_get_boxed(value))) return; - - nm_strvarray_set_strv(&p_val->arr, v); goto out_notify; - } default: goto out_fail; } @@ -1281,7 +1296,7 @@ _nm_setting_property_to_dbus_fcn_direct(_NM_SETT_INFO_PROP_TO_DBUS_FCN_ARGS _nm_ (const NMValueStrv *) _nm_setting_get_private_field(setting, sett_info, property_info); if (!val->arr) return NULL; - if (val->arr->len == 0) { + if (!property_info->direct_strv_preserve_empty && val->arr->len == 0) { /* This property does not treat empty strv arrays special. No need * to export the value on D-Bus. */ return NULL; @@ -1650,7 +1665,6 @@ _nm_setting_property_from_dbus_fcn_direct(_NM_SETT_INFO_PROP_FROM_DBUS_FCN_ARGS } case NM_VALUE_TYPE_STRV: { - NMValueStrv *p_val; gs_free const char **ss = NULL; gsize ss_len; @@ -1661,13 +1675,10 @@ _nm_setting_property_from_dbus_fcn_direct(_NM_SETT_INFO_PROP_FROM_DBUS_FCN_ARGS ss = g_variant_get_strv(value, &ss_len); nm_assert(ss_len <= G_MAXUINT); + nm_assert(NM_PTRARRAY_LEN(ss) == ss_len); - p_val = _nm_setting_get_private_field(setting, sett_info, property_info); - - if (nm_strvarray_equal_strv(p_val->arr, ss, ss_len)) + if (!_property_direct_set_strv(sett_info, property_info, setting, ss)) goto out_unchanged; - - nm_strvarray_set_strv(&p_val->arr, ss); goto out_notify; } default: @@ -2590,11 +2601,13 @@ _nm_setting_property_compare_fcn_direct(_NM_SETT_INFO_PROP_COMPARE_FCN_ARGS _nm_ const GArray *a = v_a->arr; const GArray *b = v_b->arr; - /* NULL and empty are treated identical. Coerce to NULL. */ - if (a && a->len == 0) - a = NULL; - if (b && b->len == 0) - b = NULL; + if (!property_info->direct_strv_preserve_empty) { + /* NULL and empty are treated identical. Coerce to NULL. */ + if (a && a->len == 0) + a = NULL; + if (b && b->len == 0) + b = NULL; + } return nm_strvarray_equal(a, b); } default: diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index 53a7acd2fc..e9aa99819f 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -4736,6 +4736,8 @@ test_setting_metadata(void) g_assert(sip->param_spec); g_assert(!NM_FLAGS_HAS(sip->param_spec->flags, NM_SETTING_PARAM_SECRET)); } + if (sip->direct_strv_preserve_empty) + g_assert(sip->property_type->direct_type == NM_VALUE_TYPE_STRV); if (sip->direct_set_string_mac_address_len != 0) { g_assert(NM_IN_SET(sip->property_type, diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index 1679a724f2..2d42025a61 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -818,6 +818,14 @@ struct _NMSettInfoProperty { /* Whether the string property is implemented as a (downcast) NMRefString. */ bool direct_string_is_refstr : 1; + /* Usually, for strv arrays (NM_VALUE_TYPE_STRV, NMValueStrv) there is little + * difference between NULL/unset and empty arrays. E.g. g_object_get() will + * return NULL and never an empty strv array. + * + * By setting this flag, this property treats a NULL array different from + * an empty array. */ + bool direct_strv_preserve_empty : 1; + /* Usually, properties that are set to the default value for the GParamSpec * are not serialized to GVariant (and NULL is returned by to_dbus_data(). * Set this flag to force always converting the property even if the value From cf0b482f93956d4c97ea024a00a48d35783dab66 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 15 Nov 2023 20:16:18 +0100 Subject: [PATCH 7/7] libnm: implement "{ipv4,ipv6}.dns-options" as direct STRV property "nm_sett_info_propert_type_direct_strv" is the way, now STRV properties should be implemented. Adjust the "dns-options" property.. --- src/core/dns/nm-dns-manager.c | 2 +- src/libnm-core-impl/nm-setting-ip-config.c | 125 +++++++++------------ src/libnm-core-impl/nm-setting-private.h | 2 +- src/libnm-core-impl/nm-utils.c | 21 +++- src/libnm-core-impl/tests/test-general.c | 46 ++++---- src/libnm-core-intern/nm-core-internal.h | 3 +- 6 files changed, 96 insertions(+), 103 deletions(-) diff --git a/src/core/dns/nm-dns-manager.c b/src/core/dns/nm-dns-manager.c index 86d65c0510..8f87fec1ba 100644 --- a/src/core/dns/nm-dns-manager.c +++ b/src/core/dns/nm-dns-manager.c @@ -543,7 +543,7 @@ add_string_item(GPtrArray *array, const char *str, gboolean dup) static void add_dns_option_item(GPtrArray *array, const char *str) { - if (_nm_utils_dns_option_find_idx(array, str) < 0) + if (_nm_utils_dns_option_find_idx((const char *const *) array->pdata, array->len, str) < 0) g_ptr_array_add(array, g_strdup(str)); } diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 1cae1e4447..c55babaf20 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -4345,6 +4345,12 @@ nm_setting_ip_config_clear_dns_searches(NMSettingIPConfig *setting) _notify(setting, PROP_DNS_SEARCH); } +static gssize +_dns_option_find_idx_garray(const GArray *arr, const char *option) +{ + return _nm_utils_dns_option_find_idx(nm_g_array_data(arr), nm_g_array_len(arr), option); +} + /** * nm_setting_ip_config_get_num_dns_options: * @setting: the #NMSettingIPConfig @@ -4356,13 +4362,9 @@ nm_setting_ip_config_clear_dns_searches(NMSettingIPConfig *setting) guint nm_setting_ip_config_get_num_dns_options(NMSettingIPConfig *setting) { - NMSettingIPConfigPrivate *priv; - g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), 0); - priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - - return priv->dns_options ? priv->dns_options->len : 0; + return nm_g_array_len(NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dns_options.arr); } /** @@ -4384,7 +4386,7 @@ nm_setting_ip_config_has_dns_options(NMSettingIPConfig *setting) { g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), 0); - return !!NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dns_options; + return !!NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dns_options.arr; } /** @@ -4392,6 +4394,8 @@ nm_setting_ip_config_has_dns_options(NMSettingIPConfig *setting) * @setting: the #NMSettingIPConfig * @idx: index number of the DNS option * + * Since 1.46, access at index "len" is allowed and returns NULL. + * * Returns: the DNS option at index @idx * * Since: 1.2 @@ -4399,15 +4403,11 @@ nm_setting_ip_config_has_dns_options(NMSettingIPConfig *setting) const char * nm_setting_ip_config_get_dns_option(NMSettingIPConfig *setting, guint idx) { - NMSettingIPConfigPrivate *priv; - g_return_val_if_fail(NM_IS_SETTING_IP_CONFIG(setting), NULL); - priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - g_return_val_if_fail(priv->dns_options, NULL); - g_return_val_if_fail(idx < priv->dns_options->len, NULL); - - return priv->dns_options->pdata[idx]; + return nm_strvarray_get_idxnull_or_greturn( + NM_SETTING_IP_CONFIG_GET_PRIVATE(setting)->dns_options.arr, + idx); } /** @@ -4427,11 +4427,11 @@ nm_setting_ip_config_next_valid_dns_option(NMSettingIPConfig *setting, guint idx priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - if (!priv->dns_options) + if (!priv->dns_options.arr) return -1; - for (; idx < priv->dns_options->len; idx++) { - if (_nm_utils_dns_option_validate(priv->dns_options->pdata[idx], + for (; idx < priv->dns_options.arr->len; idx++) { + if (_nm_utils_dns_option_validate(nm_strvarray_get_idx(priv->dns_options.arr, idx), NULL, NULL, NM_SETTING_IP_CONFIG_GET_ADDR_FAMILY(setting), @@ -4466,14 +4466,11 @@ nm_setting_ip_config_add_dns_option(NMSettingIPConfig *setting, const char *dns_ return FALSE; priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - if (!priv->dns_options) - priv->dns_options = g_ptr_array_new_with_free_func(g_free); - else { - if (_nm_utils_dns_option_find_idx(priv->dns_options, dns_option) >= 0) - return FALSE; - } - g_ptr_array_add(priv->dns_options, g_strdup(dns_option)); + if (_dns_option_find_idx_garray(priv->dns_options.arr, dns_option) >= 0) + return FALSE; + + nm_strvarray_ensure_and_add(&priv->dns_options.arr, dns_option); _notify(setting, PROP_DNS_OPTIONS); return TRUE; } @@ -4495,10 +4492,10 @@ nm_setting_ip_config_remove_dns_option(NMSettingIPConfig *setting, int idx) g_return_if_fail(NM_IS_SETTING_IP_CONFIG(setting)); priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - g_return_if_fail(priv->dns_options); - g_return_if_fail(idx >= 0 && idx < priv->dns_options->len); - g_ptr_array_remove_index(priv->dns_options, idx); + g_return_if_fail(idx >= 0 && idx < nm_g_array_len(priv->dns_options.arr)); + + nm_strvarray_remove_index(priv->dns_options.arr, idx); _notify(setting, PROP_DNS_OPTIONS); } @@ -4524,17 +4521,14 @@ nm_setting_ip_config_remove_dns_option_by_value(NMSettingIPConfig *setting, cons g_return_val_if_fail(dns_option[0] != '\0', FALSE); priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - if (!priv->dns_options) + + i = _dns_option_find_idx_garray(priv->dns_options.arr, dns_option); + if (i < 0) return FALSE; - i = _nm_utils_dns_option_find_idx(priv->dns_options, dns_option); - if (i >= 0) { - g_ptr_array_remove_index(priv->dns_options, i); - _notify(setting, PROP_DNS_OPTIONS); - return TRUE; - } - - return FALSE; + nm_strvarray_remove_index(priv->dns_options.arr, i); + _notify(setting, PROP_DNS_OPTIONS); + return TRUE; } /** @@ -4555,18 +4549,17 @@ nm_setting_ip_config_clear_dns_options(NMSettingIPConfig *setting, gboolean is_s g_return_if_fail(NM_IS_SETTING_IP_CONFIG(setting)); priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - if (!priv->dns_options) { + if (!priv->dns_options.arr) { if (!is_set) return; - priv->dns_options = g_ptr_array_new_with_free_func(g_free); + nm_strvarray_ensure(&priv->dns_options.arr); } else { - if (!is_set) { - g_ptr_array_unref(priv->dns_options); - priv->dns_options = NULL; - } else { - if (priv->dns_options->len == 0) + if (!is_set) + nm_strvarray_clear(&priv->dns_options.arr); + else { + if (priv->dns_options.arr->len == 0) return; - g_ptr_array_set_size(priv->dns_options, 0); + g_array_set_size(priv->dns_options.arr, 0); } } _notify(setting, PROP_DNS_OPTIONS); @@ -6124,9 +6117,13 @@ _nm_sett_info_property_override_create_array_ip_config(int addr_family) .direct_offset = NM_STRUCT_OFFSET_ENSURE_TYPE(NMValueStrv, NMSettingIPConfigPrivate, dns_search)); - _nm_properties_override_gobj(properties_override, - obj_properties[PROP_DNS_OPTIONS], - &nm_sett_info_propert_type_gprop_strv_oldstyle); + _nm_properties_override_gobj( + properties_override, + obj_properties[PROP_DNS_OPTIONS], + &nm_sett_info_propert_type_direct_strv, + .direct_offset = + NM_STRUCT_OFFSET_ENSURE_TYPE(NMValueStrv, NMSettingIPConfigPrivate, dns_options), + .direct_strv_preserve_empty = TRUE, ); _nm_properties_override_gobj(properties_override, obj_properties[PROP_DHCP_REJECT_SERVERS], @@ -6151,11 +6148,6 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) case PROP_DNS: g_value_take_boxed(value, _nm_utils_ptrarray_to_strv(priv->dns)); break; - case PROP_DNS_OPTIONS: - g_value_take_boxed(value, - priv->dns_options ? _nm_utils_ptrarray_to_strv(priv->dns_options) - : NULL); - break; case PROP_ADDRESSES: g_value_take_boxed(value, _nm_utils_copy_array(priv->addresses, @@ -6177,9 +6169,10 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) static void set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *pspec) { - NMSettingIPConfig *setting = NM_SETTING_IP_CONFIG(object); - NMSettingIPConfigPrivate *priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); - char **strv; + NMSettingIPConfig *setting = NM_SETTING_IP_CONFIG(object); + NMSettingIPConfigPrivate *priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(setting); + gs_unref_array GArray *array_free = NULL; + const char *const *strv; guint i; switch (prop_id) { @@ -6196,21 +6189,16 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps break; } case PROP_DNS_OPTIONS: - strv = g_value_get_boxed(value); - if (!strv) { - if (priv->dns_options) { - g_ptr_array_unref(priv->dns_options); - priv->dns_options = NULL; - } - } else { - if (priv->dns_options) - g_ptr_array_set_size(priv->dns_options, 0); - else - priv->dns_options = g_ptr_array_new_with_free_func(g_free); + strv = g_value_get_boxed(value); + array_free = g_steal_pointer(&priv->dns_options.arr); + if (strv) { + nm_strvarray_ensure(&priv->dns_options.arr); for (i = 0; strv[i]; i++) { - if (_nm_utils_dns_option_validate(strv[i], NULL, NULL, AF_UNSPEC, NULL) - && _nm_utils_dns_option_find_idx(priv->dns_options, strv[i]) < 0) - g_ptr_array_add(priv->dns_options, g_strdup(strv[i])); + const char *str = strv[i]; + + if (_nm_utils_dns_option_validate(str, NULL, NULL, AF_UNSPEC, NULL) + && _dns_option_find_idx_garray(priv->dns_options.arr, str) < 0) + nm_strvarray_add(priv->dns_options.arr, str); } } break; @@ -6256,7 +6244,6 @@ finalize(GObject *object) NMSettingIPConfigPrivate *priv = NM_SETTING_IP_CONFIG_GET_PRIVATE(self); nm_g_ptr_array_unref(priv->dns); - nm_g_ptr_array_unref(priv->dns_options); g_ptr_array_unref(priv->addresses); g_ptr_array_unref(priv->routes); nm_g_ptr_array_unref(priv->routing_rules); diff --git a/src/libnm-core-impl/nm-setting-private.h b/src/libnm-core-impl/nm-setting-private.h index e3f73fec5f..8964f6c798 100644 --- a/src/libnm-core-impl/nm-setting-private.h +++ b/src/libnm-core-impl/nm-setting-private.h @@ -176,8 +176,8 @@ struct _NMSettingIPConfigClass { typedef struct { NMValueStrv dns_search; /* array of domain name strings */ NMValueStrv dhcp_reject_servers; + NMValueStrv dns_options; /* array of DNS options */ GPtrArray *dns; /* array of IP address strings */ - GPtrArray *dns_options; /* array of DNS options */ GPtrArray *addresses; /* array of NMIPAddress */ GPtrArray *routes; /* array of NMIPRoute */ GPtrArray *routing_rules; diff --git a/src/libnm-core-impl/nm-utils.c b/src/libnm-core-impl/nm-utils.c index e59e572ef3..0dda3915ac 100644 --- a/src/libnm-core-impl/nm-utils.c +++ b/src/libnm-core-impl/nm-utils.c @@ -4790,7 +4790,8 @@ _nm_utils_dns_option_validate(const char *option, /** * _nm_utils_dns_option_find_idx: - * @array: an array of strings + * @strv: an array of strings of length @strv_len + * @strv_len: the length of @strv, or -1 for a NULL terminated strv array. * @option: a dns option string * * Searches for an option in an array of strings. The match is @@ -4800,18 +4801,28 @@ _nm_utils_dns_option_validate(const char *option, * found. */ gssize -_nm_utils_dns_option_find_idx(GPtrArray *array, const char *option) +_nm_utils_dns_option_find_idx(const char *const *strv, gssize strv_len, const char *option) { gs_free char *option_name = NULL; - guint i; + gsize l; + gsize i; + + if (strv_len >= 0) + l = strv_len; + else + l = NM_PTRARRAY_LEN(strv); + + if (l == 0) + return -1; if (!_nm_utils_dns_option_validate(option, &option_name, NULL, AF_UNSPEC, NULL)) return -1; - for (i = 0; i < array->len; i++) { + for (i = 0; i < l; i++) { + const char *str = strv[i]; gs_free char *tmp_name = NULL; - if (_nm_utils_dns_option_validate(array->pdata[i], &tmp_name, NULL, AF_UNSPEC, NULL)) { + if (_nm_utils_dns_option_validate(str, &tmp_name, NULL, AF_UNSPEC, NULL)) { if (nm_streq(tmp_name, option_name)) return i; } diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index a873861871..e34cc8c282 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -5375,10 +5375,8 @@ test_setting_ip4_changed_signal(void) g_object_get(s_ip4, NM_SETTING_IP_CONFIG_DNS_OPTIONS, &strv, NULL); g_assert_null(strv); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(priv->dns_options)); g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 0)); - g_test_assert_expected_messages(); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(priv->dns_options)); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(_idx <= _len)); g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 1)); g_test_assert_expected_messages(); @@ -5394,10 +5392,8 @@ test_setting_ip4_changed_signal(void) nm_clear_pointer(&strv, g_strfreev); g_assert_cmpstr(nm_setting_ip_config_get_dns_option(s_ip4, 0), ==, "debug"); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 1)); - g_test_assert_expected_messages(); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(_idx <= _len)); g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 2)); g_test_assert_expected_messages(); @@ -5411,14 +5407,13 @@ test_setting_ip4_changed_signal(void) g_assert_cmpstr(strv[0], ==, NULL); nm_clear_pointer(&strv, g_strfreev); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 0)); - g_test_assert_expected_messages(); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < priv->dns_options->len)); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(_idx <= _len)); g_assert_null(nm_setting_ip_config_get_dns_option(s_ip4, 1)); g_test_assert_expected_messages(); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx >= 0 && idx < priv->dns_options->len)); + NMTST_EXPECT_LIBNM_CRITICAL( + NMTST_G_RETURN_MSG(idx >= 0 && idx < nm_g_array_len(priv->dns_options.arr))); ASSERT_UNCHANGED(nm_setting_ip_config_remove_dns_option(s_ip4, 1)); g_test_assert_expected_messages(); @@ -8878,24 +8873,23 @@ test_nm_utils_dns_option_validate(void) static void test_nm_utils_dns_option_find_idx(void) { - GPtrArray *options; + const char *const options[] = { + "debug", + "timeout:5", + "edns0", + }; - options = g_ptr_array_new(); +#define _find_idx(options, option) \ + _nm_utils_dns_option_find_idx((options), G_N_ELEMENTS(options), ("" option "")) - g_ptr_array_add(options, "debug"); - g_ptr_array_add(options, "timeout:5"); - g_ptr_array_add(options, "edns0"); - - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, "debug"), ==, 0); - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, "debug:1"), ==, 0); - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, "timeout"), ==, 1); - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, "timeout:5"), ==, 1); - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, "timeout:2"), ==, 1); - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, "edns0"), ==, 2); - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, "rotate"), ==, -1); - g_assert_cmpint(_nm_utils_dns_option_find_idx(options, ""), ==, -1); - - g_ptr_array_free(options, TRUE); + g_assert_cmpint(_find_idx(options, "debug"), ==, 0); + g_assert_cmpint(_find_idx(options, "debug:1"), ==, 0); + g_assert_cmpint(_find_idx(options, "timeout"), ==, 1); + g_assert_cmpint(_find_idx(options, "timeout:5"), ==, 1); + g_assert_cmpint(_find_idx(options, "timeout:2"), ==, 1); + g_assert_cmpint(_find_idx(options, "edns0"), ==, 2); + g_assert_cmpint(_find_idx(options, "rotate"), ==, -1); + g_assert_cmpint(_find_idx(options, ""), ==, -1); } /*****************************************************************************/ diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index 2d42025a61..673348c640 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -413,7 +413,8 @@ gboolean _nm_utils_dns_option_validate(const char *option, long *out_value, int addr_family, const NMUtilsDNSOptionDesc *option_descs); -gssize _nm_utils_dns_option_find_idx(GPtrArray *array, const char *option); + +gssize _nm_utils_dns_option_find_idx(const char *const *strv, gssize strv_len, const char *option); int nm_setting_ip_config_next_valid_dns_option(NMSettingIPConfig *setting, guint idx);