On 8/2/22 15:56, Mohammad Heib wrote: > HI Dumitru, > thank you for reviewing the patch, > > > On Tue, Aug 2, 2022 at 1:27 PM Dumitru Ceara <[email protected]> wrote: > >> On 7/19/22 15:33, Mohammad Heib wrote: >>> ovn-northd re-create sbrec row for lport of type >>> virtual when the type explicitly updated in NBDB. >>> >>> This approach was applied to handle container lport >>> type updates and avoid handling issues in the ovn-controller, >>> this patch expanded that approach to handle lport of type >>> virtual in the same way. >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2099288 >>> Fixes: cd3b685043fa (northd: handle container lport type update) >>> Signed-off-by: Mohammad Heib <[email protected]> >>> --- >> >> Hi Mohammad, >> >> Just a small comment below; with that addressed feel free to add my ack >> to v2: >> >> Acked-by: Dumitru Ceara <[email protected]> >> >>> northd/northd.c | 31 ++++++++++++++++++------------- >>> 1 file changed, 18 insertions(+), 13 deletions(-) >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index d31cb1688..d587bc98d 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -1856,9 +1856,9 @@ localnet_can_learn_mac(const struct >> nbrec_logical_switch_port *nbsp) >>> static bool >>> lsp_is_type_changed(const struct sbrec_port_binding *sb, >>> const struct nbrec_logical_switch_port *nbsp, >>> - bool *is_old_container_lport) >>> + bool *update_sbrec) >>> { >>> - *is_old_container_lport = false; >>> + *update_sbrec = false; >>> if (!sb || !nbsp) { >>> return false; >>> } >>> @@ -1870,13 +1870,19 @@ lsp_is_type_changed(const struct >> sbrec_port_binding *sb, >>> */ >>> if ((!sb->parent_port && nbsp->parent_name) || >>> (sb->parent_port && !nbsp->parent_name)) { >>> - *is_old_container_lport = true; >>> + *update_sbrec = true; >>> return true; >>> } else { >>> return false; >>> } >>> } >>> >>> + /* Cover cases where port changed to/from virtual port */ >>> + if ((sb->type[0] && !strcmp(sb->type, "virtual"))|| >>> + (nbsp->type[0] && !strcmp(nbsp->type, "virtual"))) { >> >> There's no need to check for sb->type[0] and nbsp->type[0]. >> > actually, i want to make sure that I'm not doing strcmp with VIF port(null > type) that will lead to undefined behavior in strcmp. >
Well, "sb->type[0]" is still undefined behavior if sb->type is NULL. In any case, sb->type and nbsp->type can never be NULL. The IDL code ensures the default to be "". >> >> Also, there's a missing space before "||". >> > i added space to V2 > Thanks! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
