From ecf85fd50faa5e408a279c46efd3d8bb2dc59e94 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 Oct 2017 14:10:43 +0200 Subject: [PATCH 1/3] ifcfg-rh/tests: test nm_connection_diff() not showing difference for empty generic setting --- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 164 ++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 3c4d76726f..5d4c43b4be 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -4161,6 +4161,169 @@ test_write_wired_static (void) nmtst_assert_connection_equals (connection, FALSE, reread, FALSE); } +static void +test_write_wired_static_with_generic (void) +{ + nmtst_auto_unlinkfile char *testfile = NULL; + nmtst_auto_unlinkfile char *route6file = NULL; + gs_unref_object NMConnection *connection = NULL; + gs_unref_object NMConnection *reread = NULL; + NMSettingConnection *s_con; + NMSettingWired *s_wired; + NMSettingIPConfig *s_ip4, *reread_s_ip4; + NMSettingIPConfig *s_ip6, *reread_s_ip6; + NMIPAddress *addr; + NMIPAddress *addr6; + NMIPRoute *route6; + GError *error = NULL; + + connection = nm_simple_connection_new (); + + /* Connection setting */ + s_con = (NMSettingConnection *) nm_setting_connection_new (); + nm_connection_add_setting (connection, NM_SETTING (s_con)); + + g_object_set (s_con, + NM_SETTING_CONNECTION_ID, "Test Write Wired Static", + NM_SETTING_CONNECTION_UUID, nm_utils_uuid_generate_a (), + NM_SETTING_CONNECTION_AUTOCONNECT, TRUE, + NM_SETTING_CONNECTION_AUTOCONNECT_RETRIES, 1, + NM_SETTING_CONNECTION_TYPE, NM_SETTING_WIRED_SETTING_NAME, + NULL); + + /* Wired setting */ + s_wired = (NMSettingWired *) nm_setting_wired_new (); + nm_connection_add_setting (connection, NM_SETTING (s_wired)); + + g_object_set (s_wired, + NM_SETTING_WIRED_MAC_ADDRESS, "31:33:33:37:be:cd", + NM_SETTING_WIRED_MTU, (guint32) 1492, + NULL); + + /* IP4 setting */ + s_ip4 = (NMSettingIPConfig *) nm_setting_ip4_config_new (); + nm_connection_add_setting (connection, NM_SETTING (s_ip4)); + + g_object_set (s_ip4, + NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_MANUAL, + NM_SETTING_IP_CONFIG_MAY_FAIL, TRUE, + NM_SETTING_IP_CONFIG_GATEWAY, "1.1.1.1", + NM_SETTING_IP_CONFIG_ROUTE_METRIC, (gint64) 204, + NULL); + + addr = nm_ip_address_new (AF_INET, "1.1.1.3", 24, &error); + g_assert_no_error (error); + nm_setting_ip_config_add_address (s_ip4, addr); + nm_ip_address_unref (addr); + + addr = nm_ip_address_new (AF_INET, "1.1.1.5", 24, &error); + g_assert_no_error (error); + nm_setting_ip_config_add_address (s_ip4, addr); + nm_ip_address_unref (addr); + + nm_setting_ip_config_add_dns (s_ip4, "4.2.2.1"); + nm_setting_ip_config_add_dns (s_ip4, "4.2.2.2"); + + nm_setting_ip_config_add_dns_search (s_ip4, "foobar.com"); + nm_setting_ip_config_add_dns_search (s_ip4, "lab.foobar.com"); + + /* IP6 setting */ + s_ip6 = (NMSettingIPConfig *) nm_setting_ip6_config_new (); + nm_connection_add_setting (connection, NM_SETTING (s_ip6)); + + g_object_set (s_ip6, + NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_MANUAL, + NM_SETTING_IP_CONFIG_MAY_FAIL, TRUE, + NM_SETTING_IP_CONFIG_ROUTE_METRIC, (gint64) 206, + NULL); + + /* Add addresses */ + addr6 = nm_ip_address_new (AF_INET6, "1003:1234:abcd::1", 11, &error); + g_assert_no_error (error); + nm_setting_ip_config_add_address (s_ip6, addr6); + nm_ip_address_unref (addr6); + + addr6 = nm_ip_address_new (AF_INET6, "2003:1234:abcd::2", 22, &error); + g_assert_no_error (error); + nm_setting_ip_config_add_address (s_ip6, addr6); + nm_ip_address_unref (addr6); + + addr6 = nm_ip_address_new (AF_INET6, "3003:1234:abcd::3", 33, &error); + g_assert_no_error (error); + nm_setting_ip_config_add_address (s_ip6, addr6); + nm_ip_address_unref (addr6); + + /* Add routes */ + route6 = nm_ip_route_new (AF_INET6, + "2222:aaaa:bbbb:cccc::", 64, + "2222:aaaa:bbbb:cccc:dddd:eeee:5555:6666", 99, &error); + g_assert_no_error (error); + nm_setting_ip_config_add_route (s_ip6, route6); + nm_ip_route_unref (route6); + + route6 = nm_ip_route_new (AF_INET6, "::", 128, "2222:aaaa::9999", 1, &error); + g_assert_no_error (error); + nm_ip_route_set_attribute (route6, NM_IP_ROUTE_ATTRIBUTE_CWND, g_variant_new_uint32 (100)); + nm_ip_route_set_attribute (route6, NM_IP_ROUTE_ATTRIBUTE_MTU, g_variant_new_uint32 (1280)); + nm_ip_route_set_attribute (route6, NM_IP_ROUTE_ATTRIBUTE_LOCK_CWND, g_variant_new_boolean (TRUE)); + nm_ip_route_set_attribute (route6, NM_IP_ROUTE_ATTRIBUTE_FROM, g_variant_new_string ("2222::bbbb/32")); + nm_ip_route_set_attribute (route6, NM_IP_ROUTE_ATTRIBUTE_SRC, g_variant_new_string ("::42")); + nm_setting_ip_config_add_route (s_ip6, route6); + nm_ip_route_unref (route6); + + /* DNS servers */ + nm_setting_ip_config_add_dns (s_ip6, "fade:0102:0103::face"); + nm_setting_ip_config_add_dns (s_ip6, "cafe:ffff:eeee:dddd:cccc:bbbb:aaaa:feed"); + + /* DNS domains */ + nm_setting_ip_config_add_dns_search (s_ip6, "foobar6.com"); + nm_setting_ip_config_add_dns_search (s_ip6, "lab6.foobar.com"); + + nm_connection_add_setting (connection, nm_setting_generic_new ()); + + nmtst_assert_connection_verifies (connection); + + _writer_new_connection_FIXME (connection, + TEST_SCRATCH_DIR "/network-scripts/", + &testfile); + route6file = utils_get_route6_path (testfile); + + reread = _connection_from_file (testfile, NULL, TYPE_ETHERNET, NULL); + + /* FIXME: currently DNS domains from IPv6 setting are stored in 'DOMAIN' key in ifcfg-file + * However after re-reading they are dropped into IPv4 setting. + * So, in order to comparison succeeded, move DNS domains back to IPv6 setting. + */ + reread_s_ip4 = nm_connection_get_setting_ip4_config (reread); + reread_s_ip6 = nm_connection_get_setting_ip6_config (reread); + nm_setting_ip_config_add_dns_search (reread_s_ip6, nm_setting_ip_config_get_dns_search (reread_s_ip4, 2)); + nm_setting_ip_config_add_dns_search (reread_s_ip6, nm_setting_ip_config_get_dns_search (reread_s_ip4, 3)); + nm_setting_ip_config_remove_dns_search (reread_s_ip4, 3); + nm_setting_ip_config_remove_dns_search (reread_s_ip4, 2); + + g_assert_cmpint (nm_setting_ip_config_get_route_metric (reread_s_ip4), ==, 204); + g_assert_cmpint (nm_setting_ip_config_get_route_metric (reread_s_ip6), ==, 206); + + nm_connection_add_setting (connection, nm_setting_proxy_new ()); + + { + gs_unref_hashtable GHashTable *diffs = NULL; + + g_assert (nm_connection_diff (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT, &diffs)); + g_assert (!diffs); + g_assert (!nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); + } + g_assert (!nm_connection_get_setting (reread, NM_TYPE_SETTING_GENERIC)); + nm_connection_add_setting (reread, nm_setting_generic_new ()); + { + gs_unref_hashtable GHashTable *diffs = NULL; + + g_assert (nm_connection_diff (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT, &diffs)); + g_assert (!diffs); + g_assert (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); + } +} + static void test_write_wired_dhcp (void) { @@ -9503,6 +9666,7 @@ int main (int argc, char **argv) g_test_add_func (TPATH "wired/read/unkwnown-ethtool-opt", test_read_wired_unknown_ethtool_opt); g_test_add_func (TPATH "wired/write/static", test_write_wired_static); + g_test_add_func (TPATH "wired/write/static-with-generic", test_write_wired_static_with_generic); g_test_add_func (TPATH "wired/write/static-ip6-only", test_write_wired_static_ip6_only); g_test_add_func (TPATH "wired/write-static-routes", test_write_wired_static_routes); g_test_add_func (TPATH "wired/read-write-static-routes-legacy", test_read_write_static_routes_legacy); From 6f94b16507642ed72e03cf56bc69b0ad87a6d7ec Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 Oct 2017 13:25:54 +0200 Subject: [PATCH 2/3] libnm: fix nm_connection_diff() for settings without properties NMSettingGeneric has no properties at all. Hence, nm_connection_diff() would report that a connection A with a generic setting and a connection B without a generic setting are equal. They are not. For empty settings, let nm_setting_diff() return also empty difference hash. --- libnm-core/nm-setting.c | 10 ++++++++++ src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 6 ++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 3e2ee3debc..b420601f9c 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1322,6 +1322,7 @@ nm_setting_diff (NMSetting *a, NMSettingDiffResult a_result_default = NM_SETTING_DIFF_RESULT_IN_A_DEFAULT; NMSettingDiffResult b_result_default = NM_SETTING_DIFF_RESULT_IN_B_DEFAULT; gboolean results_created = FALSE; + gboolean compared_any = FALSE; g_return_val_if_fail (results != NULL, FALSE); g_return_val_if_fail (NM_IS_SETTING (a), FALSE); @@ -1370,6 +1371,8 @@ nm_setting_diff (NMSetting *a, if (strcmp (prop_spec->name, NM_SETTING_NAME) == 0) continue; + compared_any = TRUE; + if (b) { gboolean different; @@ -1427,6 +1430,13 @@ nm_setting_diff (NMSetting *a, } g_free (property_specs); + if (!compared_any && !b) { + /* special case: the setting has no properties, and the opposite + * setting @b is not given. The settings differ, and we signal that + * by returning an empty results hash. */ + return FALSE; + } + /* Don't return an empty hash table */ if (results_created && !g_hash_table_size (*results)) { g_hash_table_destroy (*results); diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 5d4c43b4be..2178201af8 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -4309,8 +4309,10 @@ test_write_wired_static_with_generic (void) { gs_unref_hashtable GHashTable *diffs = NULL; - g_assert (nm_connection_diff (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT, &diffs)); - g_assert (!diffs); + g_assert (!nm_connection_diff (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT, &diffs)); + g_assert (diffs); + g_assert (g_hash_table_size (diffs) == 1); + g_assert (g_hash_table_lookup (diffs, "generic")); g_assert (!nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT)); } g_assert (!nm_connection_get_setting (reread, NM_TYPE_SETTING_GENERIC)); From 975eeda611aaf39787538acb4019189270131eab Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 26 Oct 2017 13:49:03 +0200 Subject: [PATCH 3/3] libnm: fix the return value of nm_setting_diff() if a results hash was given Previously, nm_setting_diff() would return !(*results), that means, if the caller passed in a hash table (empty or not), the return value would always be FALSE, indicating a difference. That is not documented, and makes no sense. The return value, should solely indicate whether some difference was found. The only convenience is, if nm_setting_diff() created a hash table internally and no difference was found, it would destroy it again, without returning it to the caller. --- libnm-core/nm-connection.c | 41 +++++++++++++++++++++++--------------- libnm-core/nm-setting.c | 26 ++++++++++++++++-------- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index a3fae442ba..0296c6f9b2 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -516,7 +516,7 @@ nm_connection_compare (NMConnection *a, } -static void +static gboolean diff_one_connection (NMConnection *a, NMConnection *b, NMSettingCompareFlags flags, @@ -526,6 +526,7 @@ diff_one_connection (NMConnection *a, NMConnectionPrivate *priv = NM_CONNECTION_GET_PRIVATE (a); GHashTableIter iter; NMSetting *a_setting = NULL; + gboolean diff_found = FALSE; g_hash_table_iter_init (&iter, priv->settings); while (g_hash_table_iter_next (&iter, NULL, (gpointer) &a_setting)) { @@ -541,11 +542,14 @@ diff_one_connection (NMConnection *a, if (results) new_results = FALSE; - if (!nm_setting_diff (a_setting, b_setting, flags, invert_results, &results)) { - if (new_results) - g_hash_table_insert (diffs, g_strdup (setting_name), results); - } + if (!nm_setting_diff (a_setting, b_setting, flags, invert_results, &results)) + diff_found = TRUE; + + if (new_results && results) + g_hash_table_insert (diffs, g_strdup (setting_name), results); } + + return diff_found; } /** @@ -574,12 +578,11 @@ nm_connection_diff (NMConnection *a, GHashTable **out_settings) { GHashTable *diffs; + gboolean diff_found = FALSE; g_return_val_if_fail (NM_IS_CONNECTION (a), FALSE); - g_return_val_if_fail (out_settings != NULL, FALSE); - g_return_val_if_fail (*out_settings == NULL, FALSE); - if (b) - g_return_val_if_fail (NM_IS_CONNECTION (b), FALSE); + g_return_val_if_fail (!out_settings || !*out_settings, FALSE); + g_return_val_if_fail (!b || NM_IS_CONNECTION (b), FALSE); if (a == b) return TRUE; @@ -587,16 +590,22 @@ nm_connection_diff (NMConnection *a, diffs = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify) g_hash_table_destroy); /* Diff A to B, then B to A to capture keys in B that aren't in A */ - diff_one_connection (a, b, flags, FALSE, diffs); - if (b) - diff_one_connection (b, a, flags, TRUE, diffs); + if (diff_one_connection (a, b, flags, FALSE, diffs)) + diff_found = TRUE; + if ( b + && diff_one_connection (b, a, flags, TRUE, diffs)) + diff_found = TRUE; - if (g_hash_table_size (diffs) == 0) + nm_assert (diff_found == (g_hash_table_size (diffs) != 0)); + + if (g_hash_table_size (diffs) == 0) { g_hash_table_destroy (diffs); - else - *out_settings = diffs; + diffs = NULL; + } - return *out_settings ? FALSE : TRUE; + NM_SET_OUT (out_settings, diffs); + + return !diff_found; } NMSetting * diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index b420601f9c..2f282db6aa 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1323,6 +1323,7 @@ nm_setting_diff (NMSetting *a, NMSettingDiffResult b_result_default = NM_SETTING_DIFF_RESULT_IN_B_DEFAULT; gboolean results_created = FALSE; gboolean compared_any = FALSE; + gboolean diff_found = FALSE; g_return_val_if_fail (results != NULL, FALSE); g_return_val_if_fail (NM_IS_SETTING (a), FALSE); @@ -1421,6 +1422,7 @@ nm_setting_diff (NMSetting *a, if (r != NM_SETTING_DIFF_RESULT_UNKNOWN) { void *p; + diff_found = TRUE; if (g_hash_table_lookup_extended (*results, prop_spec->name, NULL, &p)) { if ((r & GPOINTER_TO_UINT (p)) != r) g_hash_table_insert (*results, g_strdup (prop_spec->name), GUINT_TO_POINTER (r | GPOINTER_TO_UINT (p))); @@ -1434,16 +1436,24 @@ nm_setting_diff (NMSetting *a, /* special case: the setting has no properties, and the opposite * setting @b is not given. The settings differ, and we signal that * by returning an empty results hash. */ + diff_found = TRUE; + } + + if (diff_found) { + /* if there is a difference, we always return FALSE. It also means, we might + * have allocated a new @results hash, and return if to the caller. */ return FALSE; + } else { + if (results_created) { + /* the allocated hash is unused. Clear it again. */ + g_hash_table_destroy (*results); + *results = NULL; + } else { + /* we found no diff, and return false. However, the input + * @result is returned unmodified. */ + } + return TRUE; } - - /* Don't return an empty hash table */ - if (results_created && !g_hash_table_size (*results)) { - g_hash_table_destroy (*results); - *results = NULL; - } - - return !(*results); } #define CMP_AND_RETURN(n_a, n_b, name) \