From 64b1b2f453de0ab5fac2b781117b99f64a676a88 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 27 Oct 2022 22:10:28 +0200 Subject: [PATCH 1/5] libnm/tests: use g_assert_cmpint() in ensure_diffs() test Just gives a better failure message. These checks fail often, when new code gets added. --- src/libnm-core-impl/tests/test-general.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index 9317451daf..3a04f1a514 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -3897,7 +3897,7 @@ ensure_diffs(GHashTable *diffs, const DiffSetting *check, gsize n_check) { guint i; - g_assert(g_hash_table_size(diffs) == n_check); + g_assert_cmpint(g_hash_table_size(diffs), ==, n_check); /* Loop through the settings */ for (i = 0; i < n_check; i++) { @@ -3910,14 +3910,14 @@ ensure_diffs(GHashTable *diffs, const DiffSetting *check, gsize n_check) /* Get the number of keys to check */ while (check[i].keys[z].key_name) z++; - g_assert(g_hash_table_size(setting_hash) == z); + g_assert_cmpint(g_hash_table_size(setting_hash), ==, z); /* Now compare the actual keys */ for (z = 0; check[i].keys[z].key_name; z++) { NMSettingDiffResult result; result = GPOINTER_TO_UINT(g_hash_table_lookup(setting_hash, check[i].keys[z].key_name)); - g_assert(result == check[i].keys[z].result); + g_assert_cmpint(result, ==, check[i].keys[z].result); } } } From 6414b016a7440f9e10fc90fcb4f71dad8890cb87 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 27 Oct 2022 21:30:17 +0200 Subject: [PATCH 2/5] libnm/tests: test comparing "ipv[46].dns" properties --- src/libnm-core-impl/tests/test-setting.c | 80 ++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index 85d549eb76..b9ba975c28 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -5068,6 +5068,84 @@ test_6lowpan_1(void) /*****************************************************************************/ +static void +test_settings_dns(void) +{ + int i_run; + + for (i_run = 0; i_run < 10; i_run++) { + gs_unref_object NMConnection *con1 = NULL; + gs_unref_object NMConnection *con2 = NULL; + int IS_IPv4; + guint n_dns; + guint i; + gboolean same = TRUE; + + con1 = + nmtst_create_minimal_connection("test-dns", NULL, NM_SETTING_WIRED_SETTING_NAME, NULL); + nmtst_connection_normalize(con1); + + con2 = nmtst_connection_duplicate_and_normalize(con1); + + nmtst_assert_connection_equals(con1, nmtst_get_rand_bool(), con2, nmtst_get_rand_bool()); + + for (IS_IPv4 = 1; IS_IPv4 >= 0; IS_IPv4--) { + const char *nameservers[2][7] = { + [0] = + { + "11:22::b:0", + "11:22::b:1#hello1", + "11:22::b:2", + "11:22::b:3#hello2", + "11:22::b:4", + "11:22::b:5", + "bogus6", + }, + [1] = + { + "1.1.1.0", + "1.1.1.1#foo1", + "1.1.1.2", + "1.1.1.3#foo2", + "1.1.1.4", + "1.1.1.5", + "bogus4", + }, + }; + GType gtype = IS_IPv4 ? NM_TYPE_SETTING_IP4_CONFIG : NM_TYPE_SETTING_IP6_CONFIG; + NMSettingIPConfig *s_ip1 = _nm_connection_get_setting(con1, gtype); + NMSettingIPConfig *s_ip2 = _nm_connection_get_setting(con2, gtype); + + n_dns = nmtst_get_rand_uint32() % G_N_ELEMENTS(nameservers[0]); + for (i = 0; i < n_dns; i++) { + const char *d = + nameservers[IS_IPv4][nmtst_get_rand_uint32() % G_N_ELEMENTS(nameservers[0])]; + + if (!nmtst_get_rand_one_case_in(4)) + nm_setting_ip_config_add_dns(s_ip1, d); + if (!nmtst_get_rand_one_case_in(4)) + nm_setting_ip_config_add_dns(s_ip2, d); + } + + if (nm_strv_ptrarray_cmp(_nm_setting_ip_config_get_dns_array(s_ip1), + _nm_setting_ip_config_get_dns_array(s_ip2)) + != 0) + same = FALSE; + } + + _nm_utils_is_manager_process = nmtst_get_rand_bool(); + if (same) { + nmtst_assert_connection_equals(con1, FALSE, con2, FALSE); + g_assert(nm_connection_compare(con1, con2, NM_SETTING_COMPARE_FLAG_EXACT)); + } else { + //TODO: g_assert(!nm_connection_compare(con1, con2, NM_SETTING_COMPARE_FLAG_EXACT)); + } + _nm_utils_is_manager_process = FALSE; + } +} + +/*****************************************************************************/ + static void test_bond_meta(void) { @@ -5170,6 +5248,8 @@ main(int argc, char **argv) g_test_add_func("/libnm/settings/6lowpan/1", test_6lowpan_1); + g_test_add_func("/libnm/settings/dns", test_settings_dns); + g_test_add_func("/libnm/settings/sriov/vf", test_sriov_vf); g_test_add_func("/libnm/settings/sriov/vf-dup", test_sriov_vf_dup); g_test_add_func("/libnm/settings/sriov/vf-vlan", test_sriov_vf_vlan); From 0f0468b208b8e1c45d57be9b1d0b1bd2d27d5d51 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 27 Oct 2022 21:43:02 +0200 Subject: [PATCH 3/5] libnm: fix _nm_setting_property_compare_fcn_default() for "to_dbus_only_in_manager_process" property_to_dbus() gets called for two reasons. Once from _nm_setting_to_dbus(). In that case, we want to honor to_dbus_only_in_manager_process(). It gets also called from _nm_setting_property_compare_fcn_default(), with ignore_flags set. In that case, we don't want to ignore the property as the hook really wants to compare them. Fixes: c8392018ca19 ('libnm: refactor to-dbus on the client skipping to serialize legacy properties') --- src/libnm-core-impl/nm-setting.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index ebb3cb5e71..f6c969ba72 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -1783,7 +1783,8 @@ property_to_dbus(const NMSettInfoSetting *sett_info, || NM_FLAGS_HAS(property_info->param_spec->flags, G_PARAM_WRITABLE) || property_info->property_type == &nm_sett_info_propert_type_setting_name); - if (property_info->to_dbus_only_in_manager_process && !_nm_utils_is_manager_process) + if (!ignore_flags && property_info->to_dbus_only_in_manager_process + && !_nm_utils_is_manager_process) return NULL; if (property_info->param_spec && !ignore_flags From 991a20b4b27b8a000e48b1e607f0fe6303540525 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 27 Oct 2022 21:10:45 +0200 Subject: [PATCH 4/5] libnm: fix comparing "ipv[46].dns" properties nm_setting_diff() ends up calling the compare_fcn() hook. Previously, the hook for "dns" was _nm_setting_property_compare_fcn_default() and the hook for "dns-data" was _nm_setting_property_compare_fcn_ignore(). That's wrong. _nm_setting_property_compare_fcn_default() converts the property to D-Bus and compares the GVariant. However, "dns" has to_dbus_only_in_manager_process set, so it wouldn't Fixes: 63eaf168d1b8 ('libnm: add "dns-data" replacement for "ipv[46].dns" properties on D-Bus') --- src/libnm-core-impl/nm-setting-ip-config.c | 18 ++++++++++++++++-- src/libnm-core-impl/nm-setting-ip4-config.c | 2 +- src/libnm-core-impl/nm-setting-ip6-config.c | 2 +- src/libnm-core-impl/nm-setting-private.h | 2 ++ src/libnm-core-impl/tests/test-setting.c | 2 +- 5 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 4c4d09bd59..118149aab7 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -5743,6 +5743,20 @@ _nm_setting_ip_config_compare_fcn_routes(_NM_SETT_INFO_PROP_COMPARE_FCN_ARGS _nm return TRUE; } +NMTernary +_nm_setting_ip_config_compare_fcn_dns(_NM_SETT_INFO_PROP_COMPARE_FCN_ARGS _nm_nil) +{ + if (NM_FLAGS_HAS(flags, NM_SETTING_COMPARE_FLAG_INFERRABLE)) + return NM_TERNARY_DEFAULT; + + if (!set_b) + return TRUE; + + return (nm_strv_ptrarray_cmp(NM_SETTING_IP_CONFIG_GET_PRIVATE(set_a)->dns, + NM_SETTING_IP_CONFIG_GET_PRIVATE(set_b)->dns) + == 0); +} + static NMTernary compare_fcn_routing_rules(_NM_SETT_INFO_PROP_COMPARE_FCN_ARGS _nm_nil) { @@ -5972,8 +5986,8 @@ _nm_sett_info_property_override_create_array_ip_config(int addr_family) "dns-data", NM_SETT_INFO_PROPERT_TYPE_DBUS(NM_G_VARIANT_TYPE("as"), .to_dbus_fcn = dns_data_to_dbus, - .compare_fcn = _nm_setting_property_compare_fcn_ignore, - .from_dbus_fcn = dns_data_from_dbus, )); + .from_dbus_fcn = dns_data_from_dbus, + .compare_fcn = _nm_setting_property_compare_fcn_ignore, )); _nm_properties_override_gobj( properties_override, diff --git a/src/libnm-core-impl/nm-setting-ip4-config.c b/src/libnm-core-impl/nm-setting-ip4-config.c index ccc3760e06..90c850c91d 100644 --- a/src/libnm-core-impl/nm-setting-ip4-config.c +++ b/src/libnm-core-impl/nm-setting-ip4-config.c @@ -972,7 +972,7 @@ nm_setting_ip4_config_class_init(NMSettingIP4ConfigClass *klass) properties_override, g_object_class_find_property(G_OBJECT_CLASS(setting_class), NM_SETTING_IP_CONFIG_DNS), NM_SETT_INFO_PROPERT_TYPE_DBUS(NM_G_VARIANT_TYPE("au"), - .compare_fcn = _nm_setting_property_compare_fcn_default, + .compare_fcn = _nm_setting_ip_config_compare_fcn_dns, .to_dbus_fcn = ip4_dns_to_dbus, .from_dbus_fcn = ip4_dns_from_dbus, ), .to_dbus_only_in_manager_process = TRUE, diff --git a/src/libnm-core-impl/nm-setting-ip6-config.c b/src/libnm-core-impl/nm-setting-ip6-config.c index a87cfb0a28..03a599699b 100644 --- a/src/libnm-core-impl/nm-setting-ip6-config.c +++ b/src/libnm-core-impl/nm-setting-ip6-config.c @@ -981,7 +981,7 @@ nm_setting_ip6_config_class_init(NMSettingIP6ConfigClass *klass) properties_override, g_object_class_find_property(G_OBJECT_CLASS(setting_class), NM_SETTING_IP_CONFIG_DNS), NM_SETT_INFO_PROPERT_TYPE_DBUS(NM_G_VARIANT_TYPE("aay"), - .compare_fcn = _nm_setting_property_compare_fcn_default, + .compare_fcn = _nm_setting_ip_config_compare_fcn_dns, .to_dbus_fcn = ip6_dns_to_dbus, .from_dbus_fcn = ip6_dns_from_dbus, ), .to_dbus_only_in_manager_process = TRUE, diff --git a/src/libnm-core-impl/nm-setting-private.h b/src/libnm-core-impl/nm-setting-private.h index 8c674053c7..90d86e12fe 100644 --- a/src/libnm-core-impl/nm-setting-private.h +++ b/src/libnm-core-impl/nm-setting-private.h @@ -1069,6 +1069,8 @@ NMTernary _nm_setting_ip_config_compare_fcn_addresses(_NM_SETT_INFO_PROP_COMPARE NMTernary _nm_setting_ip_config_compare_fcn_routes(_NM_SETT_INFO_PROP_COMPARE_FCN_ARGS _nm_nil); +NMTernary _nm_setting_ip_config_compare_fcn_dns(_NM_SETT_INFO_PROP_COMPARE_FCN_ARGS _nm_nil); + gboolean _nm_sett_info_prop_missing_from_dbus_fcn_cloned_mac_address( _NM_SETT_INFO_PROP_MISSING_FROM_DBUS_FCN_ARGS _nm_nil); diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index b9ba975c28..50da9cae5e 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -5138,7 +5138,7 @@ test_settings_dns(void) nmtst_assert_connection_equals(con1, FALSE, con2, FALSE); g_assert(nm_connection_compare(con1, con2, NM_SETTING_COMPARE_FLAG_EXACT)); } else { - //TODO: g_assert(!nm_connection_compare(con1, con2, NM_SETTING_COMPARE_FLAG_EXACT)); + g_assert(!nm_connection_compare(con1, con2, NM_SETTING_COMPARE_FLAG_EXACT)); } _nm_utils_is_manager_process = FALSE; } From e72b1f49b3f69e1b5160148a747a2d9f9cded269 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 27 Oct 2022 21:57:56 +0200 Subject: [PATCH 5/5] libnm: minor refactoring on property_to_dbus() and add comment Add a comment. Also, restructure the check so that it is (hopefully) easier to read. --- src/libnm-core-impl/nm-setting.c | 47 ++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index f6c969ba72..045c703b23 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -1783,31 +1783,36 @@ property_to_dbus(const NMSettInfoSetting *sett_info, || NM_FLAGS_HAS(property_info->param_spec->flags, G_PARAM_WRITABLE) || property_info->property_type == &nm_sett_info_propert_type_setting_name); - if (!ignore_flags && property_info->to_dbus_only_in_manager_process - && !_nm_utils_is_manager_process) - return NULL; + if (ignore_flags) { + /* We are called from _nm_setting_property_compare_fcn_default(). We want + * to serialize the property, and ignore the flags. */ + } else { + if (property_info->to_dbus_only_in_manager_process && !_nm_utils_is_manager_process) + return NULL; - if (property_info->param_spec && !ignore_flags - && !NM_FLAGS_HAS(property_info->param_spec->flags, NM_SETTING_PARAM_TO_DBUS_IGNORE_FLAGS)) { - if (NM_FLAGS_HAS(property_info->param_spec->flags, NM_SETTING_PARAM_SECRET)) { - NMSettingSecretFlags f = NM_SETTING_SECRET_FLAG_NONE; + if (property_info->param_spec + && !NM_FLAGS_HAS(property_info->param_spec->flags, + NM_SETTING_PARAM_TO_DBUS_IGNORE_FLAGS)) { + if (NM_FLAGS_HAS(property_info->param_spec->flags, NM_SETTING_PARAM_SECRET)) { + NMSettingSecretFlags f = NM_SETTING_SECRET_FLAG_NONE; - if (NM_FLAGS_ANY(flags, - NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED - | NM_CONNECTION_SERIALIZE_WITH_SECRETS_SYSTEM_OWNED - | NM_CONNECTION_SERIALIZE_WITH_SECRETS_NOT_SAVED)) { - if (!nm_setting_get_secret_flags(setting, - property_info->param_spec->name, - &f, - NULL)) + if (NM_FLAGS_ANY(flags, + NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED + | NM_CONNECTION_SERIALIZE_WITH_SECRETS_SYSTEM_OWNED + | NM_CONNECTION_SERIALIZE_WITH_SECRETS_NOT_SAVED)) { + if (!nm_setting_get_secret_flags(setting, + property_info->param_spec->name, + &f, + NULL)) + return NULL; + } + + if (!_nm_connection_serialize_secrets(flags, f)) + return NULL; + } else { + if (!_nm_connection_serialize_non_secret(flags)) return NULL; } - - if (!_nm_connection_serialize_secrets(flags, f)) - return NULL; - } else { - if (!_nm_connection_serialize_non_secret(flags)) - return NULL; } }