network: compare all multipath route nexthops

The multipath route comparator looks up every nexthop in the first
route and hence compares each entry with itself. Compare both ordered
nexthop sets instead, including weights, as the hash function does.

Follow-up for 47420573a778e83f2bddd536cf185cd5829e8ba9

diff --git a/src/network/networkd-route-nexthop.c b/src/network/networkd-route-nexthop.c
index bebb401b3395f7e483f97f8a07155043d83af142..642cc508f05e27c6a112da47c777d5036f39a14f 100644 (file)
--- a/src/network/networkd-route-nexthop.c
+++ b/src/network/networkd-route-nexthop.c
@@ -185,9 +185,16 @@ void route_nexthops_hash_func(const Route *route, struct siphash *state) {
                 return;
 
         default: {
+                uint64_t xor_hash = 0;
                 RouteNextHop *nh;
-                ORDERED_SET_FOREACH(nh, route->nexthops)
-                        route_nexthop_hash_func(nh, state);
+                ORDERED_SET_FOREACH(nh, route->nexthops) {
+                        struct siphash nh_state = *ASSERT_PTR(state);
+
+                        route_nexthop_hash_func(nh, &nh_state);
+                        xor_hash ^= siphash24_finalize(&nh_state);
+                }
+
+                siphash24_compress_typesafe(xor_hash, state);
         }}
 }
 
@@ -211,12 +218,15 @@ int route_nexthops_compare_func(const Route *a, const Route *b) {
                 return route_nexthop_compare_func_full(&a->nexthop, &b->nexthop, /* with_weight= */ false);
 
         default: {
+                r = CMP(ordered_set_size(a->nexthops), ordered_set_size(b->nexthops));
+                if (r != 0)
+                        return r;
+
                 RouteNextHop *nh;
-                ORDERED_SET_FOREACH(nh, a->nexthops) {
-                        r = CMP(nh, (RouteNextHop*) ordered_set_get(a->nexthops, nh));
-                        if (r != 0)
-                                return r;
-                }
+                ORDERED_SET_FOREACH(nh, a->nexthops)
+                        if (!ordered_set_get(b->nexthops, nh))
+                                return 1;
+
                 return 0;
         }}
 }

diff --git a/src/network/test-networkd-conf.c b/src/network/test-networkd-conf.c
index 00791cea913354133881711e1127508bf2591008..31b89fda2fa4d0342bcec20aa9a8c191c5368130 100644 (file)
--- a/src/network/test-networkd-conf.c
+++ b/src/network/test-networkd-conf.c
@@ -6,6 +6,7 @@
 #include "networkd-address.h"
 #include "networkd-manager.h"
 #include "networkd-network.h"
+#include "networkd-route.h"
 #include "networkd-route-nexthop.h"
 #include "ordered-set.h"
 #include "set.h"
@@ -307,6 +308,47 @@ static void test_config_parse_multipath_route_verify(
         ASSERT_EQ(nh->weight, expected_weight);
 }
 
+TEST(route_nexthops_compare) {
+        _cleanup_ordered_set_free_ OrderedSet *a_nexthops = NULL, *b_nexthops = NULL;
+        Route a = { .family = AF_INET }, b = { .family = AF_INET };
+        _cleanup_set_free_ Set *routes = NULL;
+
+        ASSERT_OK_EQ(parse_mpr("10.0.0.1@1 10", &a_nexthops), 1);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.2@2 20", &a_nexthops), 1);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.1@1 10", &b_nexthops), 1);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.2@2 20", &b_nexthops), 1);
+
+        a.nexthops = a_nexthops;
+        b.nexthops = b_nexthops;
+        ASSERT_EQ(route_nexthops_compare_func(&a, &b), 0);
+
+        b_nexthops = ordered_set_free(b_nexthops);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.1@1 10", &b_nexthops), 1);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.3@2 20", &b_nexthops), 1);
+        b.nexthops = b_nexthops;
+        ASSERT_NE(route_nexthops_compare_func(&a, &b), 0);
+
+        b_nexthops = ordered_set_free(b_nexthops);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.1@1 10", &b_nexthops), 1);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.2@2 30", &b_nexthops), 1);
+        b.nexthops = b_nexthops;
+        ASSERT_NE(route_nexthops_compare_func(&a, &b), 0);
+
+        b_nexthops = ordered_set_free(b_nexthops);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.2@2 20", &b_nexthops), 1);
+        ASSERT_OK_EQ(parse_mpr("10.0.0.1@1 10", &b_nexthops), 1);
+        b.nexthops = b_nexthops;
+        ASSERT_EQ(route_nexthops_compare_func(&a, &b), 0);
+
+        ASSERT_NOT_NULL(routes = set_new(&route_hash_ops));
+        ASSERT_OK_POSITIVE(set_put(routes, &a));
+        ASSERT_PTR_EQ(set_get(routes, &b), &a);
+        ASSERT_OK_ZERO(set_put(routes, &b));
+
+        ASSERT_OK_EQ(parse_mpr("10.0.0.3@3 30", &b_nexthops), 1);
+        ASSERT_NE(route_nexthops_compare_func(&a, &b), 0);
+}
+
 TEST(config_parse_multipath_route) {
         _cleanup_ordered_set_free_ OrderedSet *nexthops = NULL;