On 5/13/20 9:39 PM, Mark Michelson wrote: > MAC addresses and IPv6 addresses use hex digits which are appropriate to > express with either uppercase or lowercase letters. strcmp() detects two > MAC or IPv6 addresses as different if theier capitalization differs. > > 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. > > For IPv6 addresses, we use a normalized string format to compare them > instead of using the raw user input or what is stored in the database. >
Hi Mark, I'm not sure if we should handle everything in this single patch but there's also the case of NAT that doesn't work properly wrt. string comparisons: $ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:AB:00::AB:1 4300:AB:00::AB:1 p1 00:00:00:00:0a:01 $ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:ab:00::AB:1 4300:AB:00::AB:1 p1 $ ovn-nbctl lr-nat-add lr0 dnat_and_snat 4200:ab:00::Ab:1 4300:ab:00::AB:1 p1 00:00:00:00:0A:01 ovn-nbctl lr-nat-list lr0 TYPE EXTERNAL_IP EXTERNAL_PORT LOGICAL_IP EXTERNAL_MAC LOGICAL_PORT dnat_and_snat 4200:AB:00::AB: 4300:AB:00::AB:1 00:00:00:00:0a:01 p1 dnat_and_snat 4200:ab:00::AB: 4300:AB:00::AB:1 00:00:00:00:0a:01 p1 dnat_and_snat 4200:ab:00::Ab: 4300:ab:00::AB:1 00:00:00:00:0A:01 p1 Regards, Dumitru > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059 > Signed-off-by: Mark Michelson <mmich...@redhat.com> > --- > northd/ovn-northd.c | 17 ++++++------ > tests/ovn.at | 9 +++++++ > utilities/ovn-nbctl.c | 60 +++++++++++++++++++++++++++++++------------ > 3 files changed, 61 insertions(+), 25 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 83e6134b0..5cd60dcd1 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,9 @@ 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)) { > + 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 +11114,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..1676d8c04 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 AEF0::1/64 BEF0::1/64 > + > +AT_CHECK([ovn-nbctl --may-exist lrp-add r1 rp1 cc:dd:ee:ee:dd:cc aef0::1/64 > bef0:0000:0000:0000::1/64]) > +AT_CLEANUP > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 02fc10c9e..95bf7f5fd 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -4614,6 +4614,23 @@ nbctl_lrp_get_gateway_chassis(struct ctl_context *ctx) > free(gcs); > } > > +static struct sset * > +lrp_network_sset(const char **networks, int n_networks) > +{ > + struct sset *network_set = xzalloc(sizeof *network_set); > + sset_init(network_set); > + for (int i = 0; i < n_networks; i++) { > + char *norm = normalize_prefix_str(networks[i]); > + if (!norm) { > + sset_destroy(network_set); > + free(network_set); > + return NULL; > + } > + sset_add_and_free(network_set, norm); > + } > + return network_set; > +} > + > static void > nbctl_lrp_add(struct ctl_context *ctx) > { > @@ -4647,6 +4664,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,23 +4696,34 @@ 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; > } > > - struct sset new_networks = SSET_INITIALIZER(&new_networks); > - for (int i = 0; i < n_networks; i++) { > - sset_add(&new_networks, networks[i]); > + struct sset *new_networks = lrp_network_sset(networks, n_networks); > + if (!new_networks) { > + ctl_error(ctx, "%s: Invalid networks configured", lrp_name); > + return; > + } > + struct sset *orig_networks = lrp_network_sset((const char > **)lrp->networks, > + lrp->n_networks); > + if (!orig_networks) { > + ctl_error(ctx, "%s: Existing port has invalid networks > configured", > + lrp_name); > + sset_destroy(new_networks); > + free(new_networks); > + return; > } > > - struct sset orig_networks = SSET_INITIALIZER(&orig_networks); > - sset_add_array(&orig_networks, lrp->networks, lrp->n_networks); > - > - bool same_networks = sset_equals(&orig_networks, &new_networks); > - sset_destroy(&orig_networks); > - sset_destroy(&new_networks); > + bool same_networks = sset_equals(orig_networks, new_networks); > + sset_destroy(orig_networks); > + free(orig_networks); > + sset_destroy(new_networks); > + free(new_networks); > if (!same_networks) { > ctl_error(ctx, "%s: port already exists with different network", > lrp_name); > @@ -4715,12 +4749,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; > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev