From 8682bb7f5f59824a46f36f9ea6ab36fd5e02ca74 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 2 Feb 2016 14:30:15 +0100 Subject: [PATCH 1/2] nmtst: introduce nmtst_setting_ip_config_add_{address,route} helpers --- shared/nm-test-utils.h | 50 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/shared/nm-test-utils.h b/shared/nm-test-utils.h index 89d32ccf8c..fcae761228 100644 --- a/shared/nm-test-utils.h +++ b/shared/nm-test-utils.h @@ -1325,6 +1325,56 @@ nmtst_ip6_config_clone (NMIP6Config *config) #endif +#ifdef NM_SETTING_IP_CONFIG_H +inline static void +nmtst_setting_ip_config_add_address (NMSettingIPConfig *s_ip, + const char *address, + guint prefix) +{ + NMIPAddress *addr; + int family; + + g_assert (s_ip); + + if (nm_utils_ipaddr_valid (AF_INET, address)) + family = AF_INET; + else if (nm_utils_ipaddr_valid (AF_INET6, address)) + family = AF_INET6; + else + g_assert (FALSE); + + addr = nm_ip_address_new (family, address, prefix, NULL); + g_assert (addr); + g_assert (nm_setting_ip_config_add_address (s_ip, addr)); + nm_ip_address_unref (addr); +} + +inline static void +nmtst_setting_ip_config_add_route (NMSettingIPConfig *s_ip, + const char *dest, + guint prefix, + const char *next_hop, + gint64 metric) +{ + NMIPRoute *route; + int family; + + g_assert (s_ip); + + if (nm_utils_ipaddr_valid (AF_INET, dest)) + family = AF_INET; + else if (nm_utils_ipaddr_valid (AF_INET6, dest)) + family = AF_INET6; + else + g_assert (FALSE); + + route = nm_ip_route_new (family, dest, prefix, next_hop, metric, NULL); + g_assert (route); + g_assert (nm_setting_ip_config_add_route (s_ip, route)); + nm_ip_route_unref (route); +} +#endif /* NM_SETTING_IP_CONFIG_H */ + #if (defined(__NM_SIMPLE_CONNECTION_H__) && defined(__NM_SETTING_CONNECTION_H__)) || (defined(NM_CONNECTION_H)) inline static NMConnection * From 08c3378f6499c7c1d863f70d06938a765b466daf Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 1 Feb 2016 14:16:30 +0100 Subject: [PATCH 2/2] core: fix matching of default metric value for static routes When NM tries to match a generated connection to a persistent one, it considers also the metric of static routes. However, if the field is set to -1 (use default value for the device) on the persistent connection, the comparison will always fail because the generated connection contains the actual value read from kernel. To fix the issue, modify check_possible_match() to deal correctly with -1 and translate it to the expected value for the current device when performing the comparison. This allows connections with static routes and default metric to properly be re-assumed when NM is restarted. https://bugzilla.redhat.com/show_bug.cgi?id=1302532 --- src/NetworkManagerUtils.c | 107 ++++++++++++++++++++++++++- src/NetworkManagerUtils.h | 2 + src/nm-manager.c | 2 + src/tests/test-general.c | 148 +++++++++++++++++++++++++++++++++----- 4 files changed, 239 insertions(+), 20 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index f549875aa2..d13158cc6f 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -2305,6 +2305,95 @@ check_ip6_method (NMConnection *orig, return allow; } +static int +route_compare (NMIPRoute *route1, NMIPRoute *route2, gint64 default_metric) +{ + gint64 r, metric1, metric2; + + r = g_strcmp0 (nm_ip_route_get_dest (route1), nm_ip_route_get_dest (route2)); + if (r) + return r; + + r = nm_ip_route_get_prefix (route1) - nm_ip_route_get_prefix (route2); + if (r) + return r > 0 ? 1 : -1; + + r = g_strcmp0 (nm_ip_route_get_next_hop (route1), nm_ip_route_get_next_hop (route2)); + if (r) + return r; + + metric1 = nm_ip_route_get_metric (route1) == -1 ? default_metric : nm_ip_route_get_metric (route1); + metric2 = nm_ip_route_get_metric (route2) == -1 ? default_metric : nm_ip_route_get_metric (route2); + + r = metric1 - metric2; + if (r) + return r > 0 ? 1 : -1; + + r = nm_ip_route_get_family (route1) - nm_ip_route_get_family (route2); + if (r) + return r > 0 ? 1 : -1; + + return 0; +} + +static int +route_ptr_compare (const void *a, const void *b) +{ + return route_compare (*(NMIPRoute **) a, *(NMIPRoute **) b, -1); +} + +static gboolean +check_ip_routes (NMConnection *orig, + NMConnection *candidate, + GHashTable *settings, + gint64 default_metric, + gboolean v4) +{ + gs_free NMIPRoute **routes1 = NULL, **routes2 = NULL; + NMSettingIPConfig *s_ip1, *s_ip2; + const char *s_name; + GHashTable *props; + guint i, num; + + s_name = v4 ? NM_SETTING_IP4_CONFIG_SETTING_NAME : + NM_SETTING_IP6_CONFIG_SETTING_NAME; + + props = check_property_in_hash (settings, + s_name, + NM_SETTING_IP_CONFIG_ROUTES); + if (!props) + return TRUE; + + s_ip1 = (NMSettingIPConfig *) nm_connection_get_setting_by_name (orig, s_name); + s_ip2 = (NMSettingIPConfig *) nm_connection_get_setting_by_name (candidate, s_name); + + if (!s_ip1 || !s_ip2) + return FALSE; + + num = nm_setting_ip_config_get_num_routes (s_ip1); + if (num != nm_setting_ip_config_get_num_routes (s_ip2)) + return FALSE; + + routes1 = g_new (NMIPRoute *, num); + routes2 = g_new (NMIPRoute *, num); + + for (i = 0; i < num; i++) { + routes1[i] = nm_setting_ip_config_get_route (s_ip1, i); + routes2[i] = nm_setting_ip_config_get_route (s_ip2, i); + } + + qsort (routes1, num, sizeof (NMIPRoute *), route_ptr_compare); + qsort (routes2, num, sizeof (NMIPRoute *), route_ptr_compare); + + for (i = 0; i < num; i++) { + if (route_compare (routes1[i], routes2[i], default_metric)) + return FALSE; + } + + remove_from_hash (settings, props, s_name, NM_SETTING_IP_CONFIG_ROUTES); + return TRUE; +} + static gboolean check_ip4_method (NMConnection *orig, NMConnection *candidate, @@ -2488,7 +2577,9 @@ static NMConnection * check_possible_match (NMConnection *orig, NMConnection *candidate, GHashTable *settings, - gboolean device_has_carrier) + gboolean device_has_carrier, + gint64 default_v4_metric, + gint64 default_v6_metric) { g_return_val_if_fail (settings != NULL, NULL); @@ -2498,6 +2589,12 @@ check_possible_match (NMConnection *orig, if (!check_ip4_method (orig, candidate, settings, device_has_carrier)) return NULL; + if (!check_ip_routes (orig, candidate, settings, default_v4_metric, TRUE)) + return NULL; + + if (!check_ip_routes (orig, candidate, settings, default_v6_metric, FALSE)) + return NULL; + if (!check_connection_interface_name (orig, candidate, settings)) return NULL; @@ -2541,6 +2638,8 @@ NMConnection * nm_utils_match_connection (GSList *connections, NMConnection *original, gboolean device_has_carrier, + gint64 default_v4_metric, + gint64 default_v6_metric, NMUtilsMatchFilterFunc match_filter_func, gpointer match_filter_data) { @@ -2557,8 +2656,10 @@ nm_utils_match_connection (GSList *connections, } if (!nm_connection_diff (original, candidate, NM_SETTING_COMPARE_FLAG_INFERRABLE, &diffs)) { - if (!best_match) - best_match = check_possible_match (original, candidate, diffs, device_has_carrier); + if (!best_match) { + best_match = check_possible_match (original, candidate, diffs, device_has_carrier, + default_v4_metric, default_v6_metric); + } if (!best_match && nm_logging_enabled (LOGL_DEBUG, LOGD_CORE)) { GString *diff_string; diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index a873629799..0cb5627f29 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -328,6 +328,8 @@ typedef gboolean (NMUtilsMatchFilterFunc) (NMConnection *connection, gpointer us NMConnection *nm_utils_match_connection (GSList *connections, NMConnection *original, gboolean device_has_carrier, + gint64 default_v4_metric, + gint64 default_v6_metric, NMUtilsMatchFilterFunc match_filter_func, gpointer match_filter_data); diff --git a/src/nm-manager.c b/src/nm-manager.c index f8d177b583..e8d73a5020 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1582,6 +1582,8 @@ get_existing_connection (NMManager *manager, NMDevice *device, gboolean *out_gen matched = NM_SETTINGS_CONNECTION (nm_utils_match_connection (connections, connection, nm_device_has_carrier (device), + nm_device_get_ip4_route_metric (device), + nm_device_get_ip6_route_metric (device), match_connection_filter, device)); if (matched) { diff --git a/src/tests/test-general.c b/src/tests/test-general.c index da68c9a97c..4d979942c0 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -205,7 +205,7 @@ test_connection_match_basic (void) copy = nm_simple_connection_new_clone (orig); connections = g_slist_append (connections, copy); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == copy); /* Now change a material property like IPv4 method and ensure matching fails */ @@ -214,7 +214,7 @@ test_connection_match_basic (void) g_object_set (G_OBJECT (s_ip4), NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL, NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == NULL); g_slist_free (connections); @@ -250,7 +250,7 @@ test_connection_match_ip6_method (void) NM_SETTING_IP_CONFIG_MAY_FAIL, TRUE, NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == copy); g_slist_free (connections); @@ -284,7 +284,7 @@ test_connection_match_ip6_method_ignore (void) NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == copy); g_slist_free (connections); @@ -318,7 +318,7 @@ test_connection_match_ip6_method_ignore_auto (void) NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == copy); g_slist_free (connections); @@ -326,7 +326,6 @@ test_connection_match_ip6_method_ignore_auto (void) g_object_unref (copy); } - static void test_connection_match_ip4_method (void) { @@ -355,11 +354,11 @@ test_connection_match_ip4_method (void) NM_SETTING_IP_CONFIG_MAY_FAIL, TRUE, NULL); - matched = nm_utils_match_connection (connections, orig, FALSE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, FALSE, 0, 0, NULL, NULL); g_assert (matched == copy); /* Ensure when carrier=true matching fails */ - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == NULL); g_slist_free (connections); @@ -393,7 +392,7 @@ test_connection_match_interface_name (void) NM_SETTING_CONNECTION_INTERFACE_NAME, NULL, NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == copy); g_slist_free (connections); @@ -430,7 +429,7 @@ test_connection_match_wired (void) NM_SETTING_WIRED_S390_NETTYPE, "qeth", NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == copy); g_slist_free (connections); @@ -462,7 +461,7 @@ test_connection_match_wired2 (void) * the connections match. It can happen if assuming VLAN devices. */ nm_connection_remove_setting (orig, NM_TYPE_SETTING_WIRED); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == copy); g_slist_free (connections); @@ -487,7 +486,7 @@ test_connection_match_cloned_mac (void) NM_SETTING_WIRED_CLONED_MAC_ADDRESS, "52:54:00:ab:db:23", NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == fuzzy); exact = nm_simple_connection_new_clone (orig); @@ -498,14 +497,14 @@ test_connection_match_cloned_mac (void) NM_SETTING_WIRED_CLONED_MAC_ADDRESS, "52:54:00:ab:db:23", NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == exact); g_object_set (G_OBJECT (s_wired), NM_SETTING_WIRED_CLONED_MAC_ADDRESS, "52:54:00:ab:db:24", NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched == fuzzy); g_slist_free (connections); @@ -565,7 +564,7 @@ test_connection_no_match_ip4_addr (void) nm_setting_ip_config_add_address (s_ip4, nm_addr); nm_ip_address_unref (nm_addr); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched != copy); g_slist_free (connections); @@ -611,7 +610,7 @@ test_connection_no_match_vlan (void) NM_SETTING_VLAN_FLAGS, 0, NULL); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched != copy); /* Check that the connections do not match if VLAN priorities differ */ @@ -621,7 +620,7 @@ test_connection_no_match_vlan (void) g_object_set (G_OBJECT (s_vlan_copy), NM_SETTING_VLAN_FLAGS, 0, NULL); nm_setting_vlan_add_priority_str (s_vlan_copy, NM_VLAN_INGRESS_MAP, "4:2"); - matched = nm_utils_match_connection (connections, orig, TRUE, NULL, NULL); + matched = nm_utils_match_connection (connections, orig, TRUE, 0, 0, NULL, NULL); g_assert (matched != copy); g_slist_free (connections); @@ -629,6 +628,118 @@ test_connection_no_match_vlan (void) g_object_unref (copy); } +static void +test_connection_match_ip4_routes1 (void) +{ + gs_unref_object NMConnection *orig = NULL, *copy = NULL; + NMConnection *matched; + gs_free_slist GSList *connections = NULL; + NMSettingIPConfig *s_ip4; + + orig = _match_connection_new (); + + s_ip4 = nm_connection_get_setting_ip4_config (orig); + g_assert (s_ip4); + g_object_set (G_OBJECT (s_ip4), + NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_MANUAL, + NULL); + + nmtst_setting_ip_config_add_address (s_ip4, "10.0.0.1", 8); + + /* Clone connection */ + copy = nm_simple_connection_new_clone (orig); + connections = g_slist_append (connections, copy); + + /* Set routes on original connection */ + nmtst_setting_ip_config_add_route (s_ip4, "172.25.16.0", 24, "10.0.0.2", -1); + nmtst_setting_ip_config_add_route (s_ip4, "172.25.17.0", 24, "10.0.0.3", 20); + + /* Set single route on cloned connection */ + s_ip4 = nm_connection_get_setting_ip4_config (copy); + g_assert (s_ip4); + nmtst_setting_ip_config_add_route (s_ip4, "172.25.17.0", 24, "10.0.0.3", 20); + + /* Try to match the connections */ + matched = nm_utils_match_connection (connections, orig, FALSE, 100, 0, NULL, NULL); + g_assert (matched == NULL); +} + +static void +test_connection_match_ip4_routes2 (void) +{ + gs_unref_object NMConnection *orig = NULL, *copy = NULL; + NMConnection *matched; + gs_free_slist GSList *connections = NULL; + NMSettingIPConfig *s_ip4; + + orig = _match_connection_new (); + + s_ip4 = nm_connection_get_setting_ip4_config (orig); + g_assert (s_ip4); + g_object_set (G_OBJECT (s_ip4), + NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_MANUAL, + NULL); + + nmtst_setting_ip_config_add_address (s_ip4, "10.0.0.1", 8); + + /* Clone connection */ + copy = nm_simple_connection_new_clone (orig); + connections = g_slist_append (connections, copy); + + /* Set routes on original connection */ + nmtst_setting_ip_config_add_route (s_ip4, "172.25.16.0", 24, "10.0.0.2", -1); + nmtst_setting_ip_config_add_route (s_ip4, "172.25.17.0", 24, "10.0.0.3", 20); + + /* Set routes on cloned connection, changing order and using explicit metrics */ + s_ip4 = nm_connection_get_setting_ip4_config (copy); + g_assert (s_ip4); + nmtst_setting_ip_config_add_route (s_ip4, "172.25.17.0", 24, "10.0.0.3", 20); + nmtst_setting_ip_config_add_route (s_ip4, "172.25.16.0", 24, "10.0.0.2", 100); + + /* Try to match the connections using different default metrics */ + matched = nm_utils_match_connection (connections, orig, FALSE, 100, 0, NULL, NULL); + g_assert (matched == copy); + matched = nm_utils_match_connection (connections, orig, FALSE, 500, 0, NULL, NULL); + g_assert (matched == NULL); +} + +static void +test_connection_match_ip6_routes (void) +{ + gs_unref_object NMConnection *orig = NULL, *copy = NULL; + NMConnection *matched; + gs_free_slist GSList *connections = NULL; + NMSettingIPConfig *s_ip6; + + orig = _match_connection_new (); + + s_ip6 = nm_connection_get_setting_ip6_config (orig); + g_assert (s_ip6); + g_object_set (G_OBJECT (s_ip6), + NM_SETTING_IP_CONFIG_METHOD, NM_SETTING_IP6_CONFIG_METHOD_MANUAL, + NULL); + + nmtst_setting_ip_config_add_address (s_ip6, "fd01::15", 64); + + /* Clone connection */ + copy = nm_simple_connection_new_clone (orig); + connections = g_slist_append (connections, copy); + + /* Set routes on original connection */ + nmtst_setting_ip_config_add_route (s_ip6, "2001:db8:a:b:0:0:0:0", 64, "fd01::16", -1); + + /* Set routes on cloned connection */ + s_ip6 = nm_connection_get_setting_ip6_config (copy); + g_assert (s_ip6); + nmtst_setting_ip_config_add_route (s_ip6, "2001:db8:a:b:0:0:0:0", 64, "fd01::16", 50); + + /* Try to match the connections */ + matched = nm_utils_match_connection (connections, orig, FALSE, 0, 100, NULL, NULL); + g_assert (matched == NULL); + matched = nm_utils_match_connection (connections, orig, FALSE, 0, 50, NULL, NULL); + g_assert (matched == copy); +} + static NMConnection * _create_connection_autoconnect (const char *id, gboolean autoconnect, int autoconnect_priority) { @@ -1136,6 +1247,9 @@ main (int argc, char **argv) g_test_add_func ("/general/connection-match/cloned_mac", test_connection_match_cloned_mac); g_test_add_func ("/general/connection-match/no-match-ip4-addr", test_connection_no_match_ip4_addr); g_test_add_func ("/general/connection-match/no-match-vlan", test_connection_no_match_vlan); + g_test_add_func ("/general/connection-match/routes/ip4/1", test_connection_match_ip4_routes1); + g_test_add_func ("/general/connection-match/routes/ip4/2", test_connection_match_ip4_routes2); + g_test_add_func ("/general/connection-match/routes/ip6", test_connection_match_ip6_routes); g_test_add_func ("/general/connection-sort/autoconnect-priority", test_connection_sort_autoconnect_priority);