rdisc: fix double-addition of gateways & routes if priority increases

If a route or gateway's priority increased, the item would be added
to the array again without removing the older entry.  At the same time
don't bother adding an item with a zero lifetime, since it will just
be removed again by the clean_* functions.

src/rdisc/nm-rdisc.c

@@ -56,27 +56,34 @@ static guint signals[LAST_SIGNAL] = { 0 };
 gboolean
 nm_rdisc_add_gateway (NMRDisc *rdisc, const NMRDiscGateway *new)
 {
-	int i;
+	int i, insert_idx = -1;
 
 	for (i = 0; i < rdisc->gateways->len; i++) {
 		NMRDiscGateway *item = &g_array_index (rdisc->gateways, NMRDiscGateway, i);
 
 		if (IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) {
+			if (new->lifetime == 0) {
+				g_array_remove_index (rdisc->gateways, i--);
+				return TRUE;
+			}
+
 			if (item->preference != new->preference) {
 				g_array_remove_index (rdisc->gateways, i--);
 				continue;
 			}
+
 			memcpy (item, new, sizeof (*new));
 			return FALSE;
 		}
 
 		/* Put before less preferable gateways. */
-		if (item->preference < new->preference)
+		if (item->preference < new->preference && insert_idx < 0)
-			break;
+			insert_idx = i;
 	}
 
-	g_array_insert_val (rdisc->gateways, i, *new);
+	if (new->lifetime)
-	return TRUE;
+		g_array_insert_val (rdisc->gateways, CLAMP (insert_idx, 0, G_MAXINT), *new);
+	return !!new->lifetime;
 }
 
 gboolean
@@ -88,9 +95,15 @@ nm_rdisc_add_address (NMRDisc *rdisc, const NMRDiscAddress *new)
 		NMRDiscAddress *item = &g_array_index (rdisc->addresses, NMRDiscAddress, i);
 
 		if (IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) {
-			gboolean changed = item->timestamp + item->lifetime  != new->timestamp + new->lifetime ||
+			gboolean changed;
-			                   item->timestamp + item->preferred != new->timestamp + new->preferred;
 
+			if (new->lifetime == 0) {
+				g_array_remove_index (rdisc->addresses, i--);
+				return TRUE;
+			}
+
+			changed = item->timestamp + item->lifetime  != new->timestamp + new->lifetime ||
+			          item->timestamp + item->preferred != new->timestamp + new->preferred;
 			*item = *new;
 			return changed;
 		}
@@ -103,34 +116,42 @@ nm_rdisc_add_address (NMRDisc *rdisc, const NMRDiscAddress *new)
 	if (rdisc->max_addresses && rdisc->addresses->len >= rdisc->max_addresses)
 		return FALSE;
 
-	g_array_insert_val (rdisc->addresses, i, *new);
+	if (new->lifetime)
-	return TRUE;
+		g_array_insert_val (rdisc->addresses, i, *new);
+	return !!new->lifetime;
 }
 
 gboolean
 nm_rdisc_add_route (NMRDisc *rdisc, const NMRDiscRoute *new)
 {
-	int i;
+	int i, insert_idx = -1;
 
 	for (i = 0; i < rdisc->routes->len; i++) {
 		NMRDiscRoute *item = &g_array_index (rdisc->routes, NMRDiscRoute, i);
 
 		if (IN6_ARE_ADDR_EQUAL (&item->network, &new->network) && item->plen == new->plen) {
+			if (new->lifetime == 0) {
+				g_array_remove_index (rdisc->routes, i--);
+				return TRUE;
+			}
+
 			if (item->preference != new->preference) {
 				g_array_remove_index (rdisc->routes, i--);
 				continue;
 			}
+
 			memcpy (item, new, sizeof (*new));
 			return FALSE;
 		}
 
 		/* Put before less preferable routes. */
-		if (item->preference < new->preference)
+		if (item->preference < new->preference && insert_idx < 0)
-			break;
+			insert_idx = i;
 	}
 
-	g_array_insert_val (rdisc->routes, i, *new);
+	if (new->lifetime)
-	return TRUE;
+		g_array_insert_val (rdisc->routes, CLAMP (insert_idx, 0, G_MAXINT), *new);
+	return !!new->lifetime;
 }
 
 gboolean
@@ -142,25 +163,21 @@ nm_rdisc_add_dns_server (NMRDisc *rdisc, const NMRDiscDNSServer *new)
 		NMRDiscDNSServer *item = &g_array_index (rdisc->dns_servers, NMRDiscDNSServer, i);
 
 		if (IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) {
-			gboolean changed;
-
 			if (new->lifetime == 0) {
 				g_array_remove_index (rdisc->dns_servers, i);
 				return TRUE;
 			}
-
+			if (item->timestamp != new->timestamp || item->lifetime != new->lifetime) {
-			changed = (item->timestamp != new->timestamp ||
+				*item = *new;
-			           item->lifetime != new->lifetime);
+				return TRUE;
-			if (changed) {
-				item->timestamp = new->timestamp;
-				item->lifetime = new->lifetime;
 			}
-			return changed;
+			return FALSE;
 		}
 	}
 
-	g_array_insert_val (rdisc->dns_servers, i, *new);
+	if (new->lifetime)
-	return TRUE;
+		g_array_insert_val (rdisc->dns_servers, i, *new);
+	return !!new->lifetime;
 }
 
 /* Copies new->domain if 'new' is added to the dns_domains list */
@@ -191,10 +208,12 @@ nm_rdisc_add_dns_domain (NMRDisc *rdisc, const NMRDiscDNSDomain *new)
 		}
 	}
 
-	g_array_insert_val (rdisc->dns_domains, i, *new);
+	if (new->lifetime) {
-	item = &g_array_index (rdisc->dns_domains, NMRDiscDNSDomain, i);
+		g_array_insert_val (rdisc->dns_domains, i, *new);
-	item->domain = g_strdup (new->domain);
+		item = &g_array_index (rdisc->dns_domains, NMRDiscDNSDomain, i);
-	return TRUE;
+		item->domain = g_strdup (new->domain);
+	}
+	return !!new->lifetime;
 }
 
 /******************************************************************/

src/rdisc/tests/test-rdisc-fake.c

@@ -262,6 +262,88 @@ test_everything (void)
 	g_main_loop_unref (data.loop);
 }
 
+static void
+test_preference_changed (NMRDisc *rdisc, NMRDiscConfigMap changed, TestData *data)
+{
+	if (data->counter == 1) {
+		g_assert_cmpint (changed, ==, NM_RDISC_CONFIG_GATEWAYS |
+			                          NM_RDISC_CONFIG_ADDRESSES |
+			                          NM_RDISC_CONFIG_ROUTES);
+		g_assert_cmpint (rdisc->gateways->len, ==, 2);
+		match_gateway (rdisc->gateways, 0, "fe80::2", data->timestamp1 + 1, 10, NM_RDISC_PREFERENCE_MEDIUM);
+		match_gateway (rdisc->gateways, 1, "fe80::1", data->timestamp1, 10, NM_RDISC_PREFERENCE_LOW);
+		g_assert_cmpint (rdisc->addresses->len, ==, 2);
+		match_address (rdisc->addresses, 0, "2001:db8:a:a::1", data->timestamp1, 10, 10);
+		match_address (rdisc->addresses, 1, "2001:db8:a:a::2", data->timestamp1 + 1, 10, 10);
+		g_assert_cmpint (rdisc->routes->len, ==, 2);
+		match_route (rdisc->routes, 0, "2001:db8:a:b::", 64, "fe80::2", data->timestamp1 + 1, 10, 10);
+		match_route (rdisc->routes, 1, "2001:db8:a:a::", 64, "fe80::1", data->timestamp1, 10, 5);
+	} else if (data->counter == 2) {
+		g_assert_cmpint (changed, ==, NM_RDISC_CONFIG_GATEWAYS |
+			                          NM_RDISC_CONFIG_ADDRESSES |
+			                          NM_RDISC_CONFIG_ROUTES);
+
+		g_assert_cmpint (rdisc->gateways->len, ==, 2);
+		match_gateway (rdisc->gateways, 0, "fe80::1", data->timestamp1 + 2, 10, NM_RDISC_PREFERENCE_HIGH);
+		match_gateway (rdisc->gateways, 1, "fe80::2", data->timestamp1 + 1, 10, NM_RDISC_PREFERENCE_MEDIUM);
+		g_assert_cmpint (rdisc->addresses->len, ==, 2);
+		match_address (rdisc->addresses, 0, "2001:db8:a:a::1", data->timestamp1 + 2, 10, 10);
+		match_address (rdisc->addresses, 1, "2001:db8:a:a::2", data->timestamp1 + 1, 10, 10);
+		g_assert_cmpint (rdisc->routes->len, ==, 2);
+		match_route (rdisc->routes, 0, "2001:db8:a:a::", 64, "fe80::1", data->timestamp1 + 2, 10, 15);
+		match_route (rdisc->routes, 1, "2001:db8:a:b::", 64, "fe80::2", data->timestamp1 + 1, 10, 10);
+
+		g_assert (nm_fake_rdisc_done (NM_FAKE_RDISC (rdisc)));
+		g_main_loop_quit (data->loop);
+	}
+
+	data->counter++;
+}
+
+static void
+test_preference (void)
+{
+	NMFakeRDisc *rdisc = rdisc_new ();
+	guint32 now = nm_utils_get_monotonic_timestamp_s ();
+	TestData data = { g_main_loop_new (NULL, FALSE), 0, 0, now };
+	guint id;
+
+	/* Test that when a low-preference and medium gateway send advertisements,
+	 * that if the low-preference gateway switches to high-preference, we do
+	 * not get duplicates in the gateway list.
+	 */
+
+	id = nm_fake_rdisc_add_ra (rdisc, 1, NM_RDISC_DHCP_LEVEL_NONE, 4, 1500);
+	g_assert (id);
+	nm_fake_rdisc_add_gateway (rdisc, id, "fe80::1", now, 10, NM_RDISC_PREFERENCE_LOW);
+	nm_fake_rdisc_add_address (rdisc, id, "2001:db8:a:a::1", now, 10, 10);
+	nm_fake_rdisc_add_route (rdisc, id, "2001:db8:a:a::", 64, "fe80::1", now, 10, 5);
+
+	id = nm_fake_rdisc_add_ra (rdisc, 1, NM_RDISC_DHCP_LEVEL_NONE, 4, 1500);
+	g_assert (id);
+	nm_fake_rdisc_add_gateway (rdisc, id, "fe80::2", ++now, 10, NM_RDISC_PREFERENCE_MEDIUM);
+	nm_fake_rdisc_add_address (rdisc, id, "2001:db8:a:a::2", now, 10, 10);
+	nm_fake_rdisc_add_route (rdisc, id, "2001:db8:a:b::", 64, "fe80::2", now, 10, 10);
+
+	id = nm_fake_rdisc_add_ra (rdisc, 1, NM_RDISC_DHCP_LEVEL_NONE, 4, 1500);
+	g_assert (id);
+	nm_fake_rdisc_add_gateway (rdisc, id, "fe80::1", ++now, 10, NM_RDISC_PREFERENCE_HIGH);
+	nm_fake_rdisc_add_address (rdisc, id, "2001:db8:a:a::1", now, 10, 10);
+	nm_fake_rdisc_add_route (rdisc, id, "2001:db8:a:a::", 64, "fe80::1", now, 10, 15);
+
+	g_signal_connect (rdisc,
+	                  NM_RDISC_CONFIG_CHANGED,
+	                  G_CALLBACK (test_preference_changed),
+	                  &data);
+
+	nm_rdisc_start (NM_RDISC (rdisc));
+	g_main_loop_run (data.loop);
+	g_assert_cmpint (data.counter, ==, 3);
+
+	g_object_unref (rdisc);
+	g_main_loop_unref (data.loop);
+}
+
 NMTST_DEFINE ();
 
 int
@@ -278,6 +360,7 @@ main (int argc, char **argv)
 
 	g_test_add_func ("/rdisc/simple", test_simple);
 	g_test_add_func ("/rdisc/everything-changed", test_everything);
+	g_test_add_func ("/rdisc/preference-changed", test_preference);
 
 	return g_test_run ();
 }