On Tue, Aug 2, 2022 at 5:00 PM Dumitru Ceara <dce...@redhat.com> wrote:
> 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 <dce...@redhat.com> 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 <mh...@redhat.com> > >>> --- > >> > >> Hi Mohammad, > >> > >> Just a small comment below; with that addressed feel free to add my ack > >> to v2: > >> > >> Acked-by: Dumitru Ceara <dce...@redhat.com> > >> > >>> 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 "". > yes you are right sb->type[0] will be indeed undefined behavior if sb->type is NULL, i submitted v2 that addresses the comments above, thank you. > > >> > >> Also, there's a missing space before "||". > >> > > i added space to V2 > > > > Thanks! > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
