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

Dumitru Ceara dceara at redhat.com
Fri Jun 28 08:24:48 UTC 2019


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.

>                      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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list