[ovs-dev] [PATCH ovn] Don't use strcmp for MAC addresses

Mark Michelson mmichels at redhat.com
Fri May 8 18:51:51 UTC 2020


strcmp() for MAC addresses will detect that MAC addresses are different
if they use different capitalization.

This fixes the issue by converting MAC strings to struct eth_addr and
comparing those instead. This specifically is done when MAC addresses
are provided via user-input. For cases where the MAC strings are not
user-generated (e.g. pinctrl put_mac_binding) the code has not been
touched.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059
Signed-off-by: Mark Michelson <mmichels at redhat.com>
---
 northd/ovn-northd.c   | 18 +++++++++---------
 tests/ovn.at          |  9 +++++++++
 utilities/ovn-nbctl.c | 16 +++++++++-------
 3 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 83e6134b0..df71b1e03 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -91,6 +91,7 @@ static bool controller_event_en;
  * defined in Service_Monitor Southbound table. Since these packets
  * all locally handled, having just one mac is good enough. */
 static char svc_monitor_mac[ETH_ADDR_STRLEN + 1];
+static struct eth_addr svc_monitor_mac_ea;
 
 /* Default probe interval for NB and SB DB connections. */
 #define DEFAULT_PROBE_INTERVAL_MSEC 5000
@@ -3314,8 +3315,10 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs,
                 ovs_assert(mon_info);
                 sbrec_service_monitor_set_options(
                     mon_info->sbrec_mon, &lb_health_check->options);
+                struct eth_addr ea;
                 if (!mon_info->sbrec_mon->src_mac ||
-                    strcmp(mon_info->sbrec_mon->src_mac, svc_monitor_mac)) {
+                    !eth_addr_from_string(mon_info->sbrec_mon->src_mac, &ea) ||
+                    !eth_addr_equals(ea, svc_monitor_mac_ea)) {
                     sbrec_service_monitor_set_src_mac(mon_info->sbrec_mon,
                                                       svc_monitor_mac);
                 }
@@ -11088,12 +11091,10 @@ ovnnb_db_run(struct northd_context *ctx,
 
     const char *monitor_mac = smap_get(&nb->options, "svc_monitor_mac");
     if (monitor_mac) {
-        struct eth_addr addr;
-
-        memset(&addr, 0, sizeof addr);
-        if (eth_addr_from_string(monitor_mac, &addr)) {
+        memset(&svc_monitor_mac_ea, 0, sizeof svc_monitor_mac_ea);
+        if (eth_addr_from_string(monitor_mac, &svc_monitor_mac_ea)) {
             snprintf(svc_monitor_mac, sizeof svc_monitor_mac,
-                     ETH_ADDR_FMT, ETH_ADDR_ARGS(addr));
+                     ETH_ADDR_FMT, ETH_ADDR_ARGS(svc_monitor_mac_ea));
         } else {
             monitor_mac = NULL;
         }
@@ -11114,10 +11115,9 @@ ovnnb_db_run(struct northd_context *ctx,
         }
 
         if (!monitor_mac) {
-            struct eth_addr addr;
-            eth_addr_random(&addr);
+            eth_addr_random(&svc_monitor_mac_ea);
             snprintf(svc_monitor_mac, sizeof svc_monitor_mac,
-                     ETH_ADDR_FMT, ETH_ADDR_ARGS(addr));
+                     ETH_ADDR_FMT, ETH_ADDR_ARGS(svc_monitor_mac_ea));
             smap_replace(&options, "svc_monitor_mac", svc_monitor_mac);
         }
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 52d994972..bcc869c61 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -19179,3 +19179,12 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [expected])
 
 OVN_CLEANUP([hv1])
 AT_CLEANUP
+
+AT_SETUP([ovn -- Case-insensitive MAC])
+ovn_start
+
+ovn-nbctl lr-add r1
+ovn-nbctl lrp-add r1 rp1 CC:DD:EE:EE:DD:CC 192.168.0.0/24
+
+AT_CHECK([ovn-nbctl --may-exist lrp-add r1 rp1 cc:dd:ee:ee:dd:cc 192.168.0.0/24])
+AT_CLEANUP
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 02fc10c9e..bf8b94734 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -4647,6 +4647,12 @@ nbctl_lrp_add(struct ctl_context *ctx)
     char **settings = (char **) &ctx->argv[n_networks + 4];
     int n_settings = ctx->argc - 4 - n_networks;
 
+    struct eth_addr ea;
+    if (!eth_addr_from_string(mac, &ea)) {
+        ctl_error(ctx, "%s: invalid mac address %s", lrp_name, mac);
+        return;
+    }
+
     const struct nbrec_logical_router_port *lrp;
     error = lrp_by_name_or_uuid(ctx, lrp_name, false, &lrp);
     if (error) {
@@ -4673,7 +4679,9 @@ nbctl_lrp_add(struct ctl_context *ctx)
             return;
         }
 
-        if (strcmp(mac, lrp->mac)) {
+        struct eth_addr lrp_ea;
+        eth_addr_from_string(lrp->mac, &lrp_ea);
+        if (!eth_addr_equals(ea, lrp_ea)) {
             ctl_error(ctx, "%s: port already exists with mac %s", lrp_name,
                       lrp->mac);
             return;
@@ -4715,12 +4723,6 @@ nbctl_lrp_add(struct ctl_context *ctx)
         return;
     }
 
-    struct eth_addr ea;
-    if (!eth_addr_from_string(mac, &ea)) {
-        ctl_error(ctx, "%s: invalid mac address %s", lrp_name, mac);
-        return;
-    }
-
     for (int i = 0; i < n_networks; i++) {
         ovs_be32 ipv4;
         unsigned int plen;
-- 
2.25.4



More information about the dev mailing list