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