| Message ID | 20200513193930.3361135-1-mmichels@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn,v2] Avoid case sensitive comparisons for MAC and IPv6 addresses | expand |
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 83 characters long (recommended limit is 79)
#158 FILE: utilities/ovn-nbctl.c:4733:
struct sset *orig_networks = lrp_network_sset((const char **)lrp->networks,
Lines checked: 197, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
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 <mmichels@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; >
On 5/14/20 4:42 AM, Dumitru Ceara wrote: > 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 Hi Dumitru, You've found a sneaky one. nbctl_lr_nat_add() attempts to get around this by normalizing the IP string of the new NAT IPs. However, it doesn't also normalize the IP string of the existing NAT IPs. > >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059 >> Signed-off-by: Mark Michelson <mmichels@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; >> >
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;
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. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1829059 Signed-off-by: Mark Michelson <mmichels@redhat.com> --- northd/ovn-northd.c | 17 ++++++------ tests/ovn.at | 9 +++++++ utilities/ovn-nbctl.c | 60 +++++++++++++++++++++++++++++++------------ 3 files changed, 61 insertions(+), 25 deletions(-)