[ovs-dev] [PATCH v2 1/3] ovn-northd: Refactor the code which sets nat_addresses

Numan Siddique nusiddiq at redhat.com
Mon Jul 1 07:46:20 UTC 2019


On Fri, Jun 28, 2019 at 1:55 PM Dumitru Ceara <dceara at redhat.com> wrote:

> On Fri, Jun 14, 2019 at 2:37 PM <nusiddiq at redhat.com> wrote:
> >
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > The present code which sets the Port_Binding.nat_addresses
> > can be simplied. This patch does this. This would help in
> > upcoming commits to set the nat_addresses column with the
> > mac and IPs of distributed logical router ports and logical
> > router ports with 'reside-on-redirect-chassis' set.
> >
> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
>
> Hi Numan,
>
> I have a couple of minor comments inline.
>
> > ---
> >  ovn/northd/ovn-northd.c | 30 +++++++++++++-----------------
> >  1 file changed, 13 insertions(+), 17 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 0b0a96a3a..d0a77293a 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -2493,23 +2493,12 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> >
> >              const char *nat_addresses = smap_get(&op->nbsp->options,
> >                                             "nat-addresses");
> > +            size_t n_nats = 0;
> > +            char **nats = NULL;
> >              if (nat_addresses && !strcmp(nat_addresses, "router")) {
> >                  if (op->peer && op->peer->od
> >                      && (chassis || op->peer->od->l3redirect_port)) {
> > -                    size_t n_nats;
> > -                    char **nats = get_nat_addresses(op->peer, &n_nats);
> > -                    if (n_nats) {
> > -                        sbrec_port_binding_set_nat_addresses(op->sb,
> > -                            (const char **) nats, n_nats);
> > -                        for (size_t i = 0; i < n_nats; i++) {
> > -                            free(nats[i]);
> > -                        }
> > -                        free(nats);
> > -                    } else {
> > -                        sbrec_port_binding_set_nat_addresses(op->sb,
> NULL, 0);
> > -                    }
> > -                } else {
> > -                    sbrec_port_binding_set_nat_addresses(op->sb, NULL,
> 0);
> > +                    nats = get_nat_addresses(op->peer, &n_nats);
> >                  }
> >              /* Only accept manual specification of ethernet address
> >               * followed by IPv4 addresses on type "l3gateway" ports. */
> > @@ -2519,15 +2508,22 @@ ovn_port_update_sbrec(struct northd_context *ctx,
> >                      static struct vlog_rate_limit rl =
> >                          VLOG_RATE_LIMIT_INIT(1, 1);
> >                      VLOG_WARN_RL(&rl, "Error extracting
> nat-addresses.");
> > -                    sbrec_port_binding_set_nat_addresses(op->sb, NULL,
> 0);
> >                  } else {
> >                      sbrec_port_binding_set_nat_addresses(op->sb,
> >
>  &nat_addresses, 1);
>
> Shouldn't this be removed? Now we call
> sbrec_port_binding_set_nat_addresses at the end of the function for
> all cases.
>
>
Thanks for pointing this out. I missed it. I have addressed it in v2.


> >                      destroy_lport_addresses(&laddrs);
> > +                    n_nats = 1;
> > +                    nats = xcalloc(1, sizeof *nats);
> > +                    nats[0] = xstrdup(nat_addresses);
>
> I guess xmalloc would be enough as we immediately initialize nats[0].
>

I personally would prefer xcalloc because of 'nats' is a double pointer. So
I have not changed in v2.

Thanks
Numan



>
> Thanks,
> Dumitru
>
> >                  }
> > -            } else {
> > -                sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
> >              }
> > +
> > +            sbrec_port_binding_set_nat_addresses(op->sb,
> > +                                                 (const char **) nats,
> n_nats);
> > +            for (size_t i = 0; i < n_nats; i++) {
> > +                free(nats[i]);
> > +            }
> > +            free(nats);
> >          }
> >          sbrec_port_binding_set_parent_port(op->sb,
> op->nbsp->parent_name);
> >          sbrec_port_binding_set_tag(op->sb, op->nbsp->tag,
> op->nbsp->n_tag);
> > --
> > 2.21.0
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list