diff mbox series

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

Message ID 20190614123622.13011-1-nusiddiq@redhat.com
State Superseded
Headers show
Series Handle GARPs for logical router port IPs | expand

Commit Message

Numan Siddique June 14, 2019, 12:36 p.m. UTC 
From: Numan Siddique <nusiddiq@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@redhat.com>
---
 ovn/northd/ovn-northd.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

Comments

Dumitru Ceara June 28, 2019, 8:24 a.m. UTC | #1 
On Fri, Jun 14, 2019 at 2:37 PM <nusiddiq@redhat.com> wrote:
>
> From: Numan Siddique <nusiddiq@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@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.

>                      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].

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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Numan Siddique July 1, 2019, 7:46 a.m. UTC | #2 
On Fri, Jun 28, 2019 at 1:55 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On Fri, Jun 14, 2019 at 2:37 PM <nusiddiq@redhat.com> wrote:
> >
> > From: Numan Siddique <nusiddiq@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@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@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

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);
                     destroy_lport_addresses(&laddrs);
+                    n_nats = 1;
+                    nats = xcalloc(1, sizeof *nats);
+                    nats[0] = xstrdup(nat_addresses);
                 }
-            } 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);