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.

>
> Also, there's a missing space before "||".
>
i added space to V2

>
> Thanks,
> Dumitru
>
> Thank you
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to