[ovs-dev] [PATCH v2] ovn-northd: Sort options in put_dhcp(v6)_opts.

[Daniele Di Proietto]

The order of the options in the packet generated by ovs-controller
depends on the hash function.  I believe that murmur hash (our default)
produces different outputs depending on the endianness of the system.
Also, if SSE4.2 is enabled at build time, we use CRC32 for hashing which
gives different results even on x86.

This causes one unit test to fail on big endian or with SSE4.2:

ovn -- dhcpv4 : 1 HV, 2 LS, 2 LSPs/LS

This commit fixes the problem in ovn-northd by always sorting dhcp
options inside the logical flow put_dhcp(v6)_opts action.

Reported-at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=840770
Suggested-by: Ben Pfaff <blp at ovn.org>
Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
---
 ovn/northd/ovn-northd.c | 25 +++++++++++++++++--------
 tests/ovn.at            |  4 ++--
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index f2dc353..c56ac79 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2075,10 +2075,15 @@ build_dhcpv4_action(struct ovn_port *op, ovs_be32 offer_ip,
     ds_put_format(options_action,
                   REGBIT_DHCP_OPTS_RESULT" = put_dhcp_opts(offerip = "
                   IP_FMT", ", IP_ARGS(offer_ip));
-    struct smap_node *node;
-    SMAP_FOR_EACH(node, &dhcpv4_options) {
+
+    /* We're not using SMAP_FOR_EACH because we want a consistent order of the
+     * options on different architectures (big or little endian, SSE4.2) */
+    const struct smap_node **sorted_opts = smap_sort(&dhcpv4_options);
+    for (size_t i = 0; i < smap_count(&dhcpv4_options); i++) {
+        const struct smap_node *node = sorted_opts[i];
         ds_put_format(options_action, "%s = %s, ", node->key, node->value);
     }
+    free(sorted_opts);
 
     ds_chomp(options_action, ' ');
     ds_chomp(options_action, ',');
@@ -2119,9 +2124,9 @@ build_dhcpv6_action(struct ovn_port *op, struct in6_addr *offer_ip,
         return false;
     }
 
+    const struct smap *options_map = &op->nbsp->dhcpv6_options->options;
     /* "server_id" should be the MAC address. */
-    const char *server_mac = smap_get(&op->nbsp->dhcpv6_options->options,
-                                      "server_id");
+    const char *server_mac = smap_get(options_map, "server_id");
     struct eth_addr ea;
     if (!server_mac || !eth_addr_from_string(server_mac, &ea)) {
         /* "server_id" should be present in the dhcpv6_options. */
@@ -2146,20 +2151,24 @@ build_dhcpv6_action(struct ovn_port *op, struct in6_addr *offer_ip,
 
     /* Check whether the dhcpv6 options should be configured as stateful.
      * Only reply with ia_addr option for dhcpv6 stateful address mode. */
-    if (!smap_get_bool(&op->nbsp->dhcpv6_options->options,
-                       "dhcpv6_stateless", false)) {
+    if (!smap_get_bool(options_map, "dhcpv6_stateless", false)) {
         char ia_addr[INET6_ADDRSTRLEN + 1];
         ipv6_string_mapped(ia_addr, offer_ip);
 
         ds_put_format(options_action, "ia_addr = %s, ", ia_addr);
     }
 
-    struct smap_node *node;
-    SMAP_FOR_EACH (node, &op->nbsp->dhcpv6_options->options) {
+    /* We're not using SMAP_FOR_EACH because we want a consistent order of the
+     * options on different architectures (big or little endian, SSE4.2) */
+    const struct smap_node **sorted_opts = smap_sort(options_map);
+    for (size_t i = 0; i < smap_count(options_map); i++) {
+        const struct smap_node *node = sorted_opts[i];
         if (strcmp(node->key, "dhcpv6_stateless")) {
             ds_put_format(options_action, "%s = %s, ", node->key, node->value);
         }
     }
+    free(sorted_opts);
+
     ds_chomp(options_action, ' ');
     ds_chomp(options_action, ',');
     ds_put_cstr(options_action, "); next;");
diff --git a/tests/ovn.at b/tests/ovn.at
index cb21210..628d3c8 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3754,7 +3754,7 @@ as hv1 ovs-ofctl dump-flows br-int
 # Send DHCPDISCOVER.
 offer_ip=`ip_to_hex 10 0 0 4`
 server_ip=`ip_to_hex 10 0 0 1`
-expected_dhcp_opts=0104ffffff0003040a00000136040a000001330400000e10
+expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
 test_dhcp 1 f00000000001 01 $offer_ip ff1000000001 $server_ip $expected_dhcp_opts
 
 # NXT_RESUMEs should be 1.
@@ -3777,7 +3777,7 @@ rm -f 2.expected
 # Send DHCPREQUEST.
 offer_ip=`ip_to_hex 10 0 0 6`
 server_ip=`ip_to_hex 10 0 0 1`
-expected_dhcp_opts=0104ffffff0003040a00000136040a000001330400000e10
+expected_dhcp_opts=330400000e100104ffffff0003040a00000136040a000001
 test_dhcp 2 f00000000002 03 $offer_ip ff1000000001 $server_ip $expected_dhcp_opts
 
 # NXT_RESUMEs should be 2.
-- 
2.10.2