platform: merge branch 'th/route-fixes-bgo741871-v2'

https://bugzilla.gnome.org/show_bug.cgi?id=741871

6 files changed, 206 insertions, 7 deletions

diff --git a/src/platform/nm-fake-platform.c b/src/platform/nm-fake-platform.c
index a5eb7d4ed3..b4bad844f0 100644
--- a/src/platform/nm-fake-platform.c
+++ b/src/platform/nm-fake-platform.c
@@ -1076,6 +1076,8 @@ ip4_route_add (NMPlatform *platform, int ifindex, NMIPConfigSource source,
 			continue;
 		if (item->plen != route.plen)
 			continue;
+		if (item->metric != metric)
+			continue;
 
 		memcpy (item, &route, sizeof (route));
 		g_signal_emit_by_name (platform, NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, ifindex, &route, NM_PLATFORM_SIGNAL_CHANGED, NM_PLATFORM_REASON_INTERNAL);
@@ -1097,6 +1099,8 @@ ip6_route_add (NMPlatform *platform, int ifindex, NMIPConfigSource source,
 	NMPlatformIP6Route route;
 	guint i;
 
+	metric = nm_utils_ip6_route_metric_normalize (metric);
+
 	memset (&route, 0, sizeof (route));
 	route.source = NM_IP_CONFIG_SOURCE_KERNEL;
 	route.ifindex = ifindex;
@@ -1116,6 +1120,8 @@ ip6_route_add (NMPlatform *platform, int ifindex, NMIPConfigSource source,
 			continue;
 		if (item->plen != route.plen)
 			continue;
+		if (item->metric != metric)
+			continue;
 
 		memcpy (item, &route, sizeof (route));
 		g_signal_emit_by_name (platform, NM_PLATFORM_SIGNAL_IP6_ROUTE_CHANGED, ifindex, &route, NM_PLATFORM_SIGNAL_CHANGED, NM_PLATFORM_REASON_INTERNAL);
@@ -1153,6 +1159,8 @@ ip6_route_get (NMPlatform *platform, int ifindex, struct in6_addr network, int p
 	NMFakePlatformPrivate *priv = NM_FAKE_PLATFORM_GET_PRIVATE (platform);
 	int i;
 
+	metric = nm_utils_ip6_route_metric_normalize (metric);
+
 	for (i = 0; i < priv->ip6_routes->len; i++) {
 		NMPlatformIP6Route *route = &g_array_index (priv->ip6_routes, NMPlatformIP6Route, i);
 
diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c
index 50dfe16c78..48b5f8e16b 100644
--- a/src/platform/nm-linux-platform.c
+++ b/src/platform/nm-linux-platform.c
@@ -1668,6 +1668,9 @@ announce_object (NMPlatform *platform, const struct nl_object *object, NMPlatfor
 		{
 			NMPlatformIP4Route route;
 
+			if (reason == _NM_PLATFORM_REASON_CACHE_CHECK_INTERNAL)
+				return;
+
 			if (!_route_match ((struct rtnl_route *) object, AF_INET, 0, FALSE)) {
 				nm_log_dbg (LOGD_PLATFORM, "skip announce unmatching IP4 route %s", to_string_ip4_route ((struct rtnl_route *) object));
 				return;
@@ -1680,6 +1683,9 @@ announce_object (NMPlatform *platform, const struct nl_object *object, NMPlatfor
 		{
 			NMPlatformIP6Route route;
 
+			if (reason == _NM_PLATFORM_REASON_CACHE_CHECK_INTERNAL)
+				return;
+
 			if (!_route_match ((struct rtnl_route *) object, AF_INET6, 0, FALSE)) {
 				nm_log_dbg (LOGD_PLATFORM, "skip announce unmatching IP6 route %s", to_string_ip6_route ((struct rtnl_route *) object));
 				return;
@@ -3919,7 +3925,7 @@ ip4_route_delete (NMPlatform *platform, int ifindex, in_addr_t network, int plen
 		 *
 		 * Instead, re-fetch the route from kernel, and if that fails, there is nothing to do.
 		 * On success, there is still a race that we might end up deleting the wrong route. */
-		if (!refresh_object (platform, (struct nl_object *) route, FALSE, NM_PLATFORM_REASON_INTERNAL)) {
+		if (!refresh_object (platform, (struct nl_object *) route, FALSE, _NM_PLATFORM_REASON_CACHE_CHECK_INTERNAL)) {
 			rtnl_route_put ((struct rtnl_route *) route);
 			return TRUE;
 		}
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 848f78e428..3f37ed6048 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -69,7 +69,10 @@ typedef enum {
 	/* Event came from the kernel. */
 	NM_PLATFORM_REASON_EXTERNAL,
 	/* Event is a result of cache checking and cleanups. */
-	NM_PLATFORM_REASON_CACHE_CHECK
+	NM_PLATFORM_REASON_CACHE_CHECK,
+
+	/* Internal reason to suppress announcing change events */
+	_NM_PLATFORM_REASON_CACHE_CHECK_INTERNAL,
 } NMPlatformReason;
 
 #define __NMPlatformObject_COMMON \
diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c
index ab40f65240..3e716bf8c9 100644
--- a/src/platform/tests/test-common.c
+++ b/src/platform/tests/test-common.c
@@ -125,6 +125,113 @@ link_callback (NMPlatform *platform, int ifindex, NMPlatformLink *received, NMPl
 		g_error ("Added/changed link not found in the local cache.");
 }
 
+gboolean
+ip4_route_exists (const char *ifname, guint32 network, int plen, guint32 metric)
+{
+	gs_free char *arg_network = NULL;
+	const char *argv[] = {
+		NULL,
+		"route",
+		"list",
+		"dev",
+		ifname,
+		"exact",
+		NULL,
+		NULL,
+	};
+	int exit_status;
+	gs_free char *std_out = NULL, *std_err = NULL;
+	char *out;
+	gboolean success;
+	gs_free_error GError *error = NULL;
+	gs_free char *metric_pattern = NULL;
+
+	g_assert (ifname && nm_utils_iface_valid_name (ifname));
+	g_assert (!strstr (ifname, " metric "));
+	g_assert (plen >= 0 && plen <= 32);
+
+	if (!NM_IS_LINUX_PLATFORM (nm_platform_get ())) {
+		/* If we don't test against linux-platform, we don't actually configure any
+		 * routes in the system. */
+		return -1;
+	}
+
+	argv[0] = nm_utils_file_search_in_paths ("ip", NULL,
+	                                         (const char *[]) { "/sbin", "/usr/sbin", NULL },
+	                                         G_FILE_TEST_IS_EXECUTABLE, NULL, NULL, NULL);
+	argv[6] = arg_network = g_strdup_printf ("%s/%d", nm_utils_inet4_ntop (network, NULL), plen);
+
+	if (!argv[0]) {
+		/* Hm. There is no 'ip' binary. Return *unknown* */
+		return -1;
+	}
+
+	success = g_spawn_sync (NULL,
+	                        (char **) argv,
+	                        (char *[]) { NULL },
+	                        0,
+	                        NULL,
+	                        NULL,
+	                        &std_out,
+	                        &std_err,
+	                        &exit_status,
+	                        &error);
+	g_assert_no_error (error);
+	g_assert (success);
+	g_assert_cmpstr (std_err, ==, "");
+	g_assert (std_out);
+
+	metric_pattern = g_strdup_printf (" metric %u", metric);
+	out = std_out;
+	while (out) {
+		char *eol = strchr (out, '\n');
+		gs_free char *line = eol ? g_strndup (out, eol - out) : g_strdup (out);
+		const char *p;
+
+		out = eol ? &eol[1] : NULL;
+		if (!line[0])
+			continue;
+
+		if (metric == 0) {
+			if (!strstr (line, " metric "))
+				return TRUE;
+		}
+		p = strstr (line, metric_pattern);
+		if (p && NM_IN_SET (p[strlen (metric_pattern)], ' ', '\0'))
+			return TRUE;
+	}
+	return FALSE;
+}
+
+void
+_assert_ip4_route_exists (const char *file, guint line, const char *func, gboolean exists, const char *ifname, guint32 network, int plen, guint32 metric)
+{
+	int ifindex;
+	gboolean exists_checked;
+
+	/* Check for existance of the route by spawning iproute2. Do this because platform
+	 * code might be entirely borked, but we expect ip-route to give a correct result.
+	 * If the ip command cannot be found, we accept this as success. */
+	exists_checked = ip4_route_exists (ifname, network, plen, metric);
+	if (exists_checked != -1 && !exists_checked != !exists) {
+		g_error ("[%s:%u] %s(): We expect the ip4 route %s/%d metric %u %s, but it %s",
+		         file, line, func,
+		         nm_utils_inet4_ntop (network, NULL), plen, metric,
+		         exists ? "to exist" : "not to exist",
+		         exists ? "doesn't" : "does");
+	}
+
+	ifindex = nm_platform_link_get_ifindex (ifname);
+	g_assert (ifindex > 0);
+	if (!nm_platform_ip4_route_exists (ifindex, network, plen, metric) != !exists) {
+		g_error ("[%s:%u] %s(): The ip4 route %s/%d metric %u %s, but platform thinks %s",
+		         file, line, func,
+		         nm_utils_inet4_ntop (network, NULL), plen, metric,
+		         exists ? "exists" : "does not exist",
+		         exists ? "it doesn't" : "it does");
+	}
+}
+
 void
 run_command (const char *format, ...)
 {
diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h
index 8f6b391266..17c4029d8c 100644
--- a/src/platform/tests/test-common.h
+++ b/src/platform/tests/test-common.h
@@ -34,6 +34,11 @@ void accept_signal (SignalData *data);
 void wait_signal (SignalData *data);
 void free_signal (SignalData *data);
 
+gboolean ip4_route_exists (const char *ifname, guint32 network, int plen, guint32 metric);
+
+void _assert_ip4_route_exists (const char *file, guint line, const char *func, gboolean exists, const char *ifname, guint32 network, int plen, guint32 metric);
+#define assert_ip4_route_exists(exists, ifname, network, plen, metric) _assert_ip4_route_exists (__FILE__, __LINE__, G_STRFUNC, exists, ifname, network, plen, metric)
+
 void link_callback (NMPlatform *platform, int ifindex, NMPlatformLink *received, NMPlatformSignalChangeType change_type, NMPlatformReason reason, SignalData *data);
 
 void run_command (const char *format, ...);
diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c
index 9ccca0a0f6..fec1e38d26 100644
--- a/src/platform/tests/test-route.c
+++ b/src/platform/tests/test-route.c
@@ -51,6 +51,75 @@ ip6_route_callback (NMPlatform *platform, int ifindex, NMPlatformIP6Route *recei
 }
 
 static void
+test_ip4_route_metric0 (void)
+{
+	int ifindex = nm_platform_link_get_ifindex (DEVICE_NAME);
+	SignalData *route_added = add_signal (NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, NM_PLATFORM_SIGNAL_ADDED, ip4_route_callback);
+	SignalData *route_changed = add_signal (NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, NM_PLATFORM_SIGNAL_CHANGED, ip4_route_callback);
+	SignalData *route_removed = add_signal (NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, NM_PLATFORM_SIGNAL_REMOVED, ip4_route_callback);
+	in_addr_t network = nmtst_inet4_from_string ("192.0.2.5"); /* from 192.0.2.0/24 (TEST-NET-1) (rfc5737) */
+	int plen = 32;
+	int metric = 22987;
+	int mss = 1000;
+
+	/* No routes initially */
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, 0);
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, metric);
+
+	/* add the first route */
+	g_assert (nm_platform_ip4_route_add (ifindex, NM_IP_CONFIG_SOURCE_USER, network, plen, INADDR_ANY, 0, metric, mss));
+	no_error ();
+	accept_signal (route_added);
+
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, 0);
+	assert_ip4_route_exists (TRUE,  DEVICE_NAME, network, plen, metric);
+
+	/* Deleting route with metric 0 does nothing */
+	g_assert (nm_platform_ip4_route_delete (ifindex, network, plen, 0));
+	no_error ();
+	g_assert (!route_removed->received);
+
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, 0);
+	assert_ip4_route_exists (TRUE,  DEVICE_NAME, network, plen, metric);
+
+	/* add the second route */
+	g_assert (nm_platform_ip4_route_add (ifindex, NM_IP_CONFIG_SOURCE_USER, network, plen, INADDR_ANY, 0, 0, mss));
+	no_error ();
+	accept_signal (route_added);
+
+	assert_ip4_route_exists (TRUE,  DEVICE_NAME, network, plen, 0);
+	assert_ip4_route_exists (TRUE,  DEVICE_NAME, network, plen, metric);
+
+	/* Delete route with metric 0 */
+	g_assert (nm_platform_ip4_route_delete (ifindex, network, plen, 0));
+	no_error ();
+	accept_signal (route_removed);
+
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, 0);
+	assert_ip4_route_exists (TRUE,  DEVICE_NAME, network, plen, metric);
+
+	/* Delete route with metric 0 again (we expect nothing to happen) */
+	g_assert (nm_platform_ip4_route_delete (ifindex, network, plen, 0));
+	no_error ();
+	g_assert (!route_removed->received);
+
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, 0);
+	assert_ip4_route_exists (TRUE,  DEVICE_NAME, network, plen, metric);
+
+	/* Delete the other route */
+	g_assert (nm_platform_ip4_route_delete (ifindex, network, plen, metric));
+	no_error ();
+	accept_signal (route_removed);
+
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, 0);
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, metric);
+
+	free_signal (route_added);
+	free_signal (route_changed);
+	free_signal (route_removed);
+}
+
+static void
 test_ip4_route (void)
 {
 	int ifindex = nm_platform_link_get_ifindex (DEVICE_NAME);
@@ -75,11 +144,11 @@ test_ip4_route (void)
 	accept_signal (route_added);
 
 	/* Add route */
-	g_assert (!nm_platform_ip4_route_exists (ifindex, network, plen, metric));
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, metric);
 	no_error ();
 	g_assert (nm_platform_ip4_route_add (ifindex, NM_IP_CONFIG_SOURCE_USER, network, plen, gateway, 0, metric, mss));
 	no_error ();
-	g_assert (nm_platform_ip4_route_exists (ifindex, network, plen, metric));
+	assert_ip4_route_exists (TRUE, DEVICE_NAME, network, plen, metric);
 	no_error ();
 	accept_signal (route_added);
 
@@ -89,11 +158,11 @@ test_ip4_route (void)
 	accept_signal (route_changed);
 
 	/* Add default route */
-	g_assert (!nm_platform_ip4_route_exists (ifindex, 0, 0, metric));
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, 0, 0, metric);
 	no_error ();
 	g_assert (nm_platform_ip4_route_add (ifindex, NM_IP_CONFIG_SOURCE_USER, 0, 0, gateway, 0, metric, mss));
 	no_error ();
-	g_assert (nm_platform_ip4_route_exists (ifindex, 0, 0, metric));
+	assert_ip4_route_exists (TRUE, DEVICE_NAME, 0, 0, metric);
 	no_error ();
 	accept_signal (route_added);
 
@@ -134,7 +203,7 @@ test_ip4_route (void)
 	/* Remove route */
 	g_assert (nm_platform_ip4_route_delete (ifindex, network, plen, metric));
 	no_error ();
-	g_assert (!nm_platform_ip4_route_exists (ifindex, network, plen, metric));
+	assert_ip4_route_exists (FALSE, DEVICE_NAME, network, plen, metric);
 	accept_signal (route_removed);
 
 	/* Remove route again */
@@ -257,4 +326,5 @@ setup_tests (void)
 
 	g_test_add_func ("/route/ip4", test_ip4_route);
 	g_test_add_func ("/route/ip6", test_ip6_route);
+	g_test_add_func ("/route/ip4_metric0", test_ip4_route_metric0);
 }