diff mbox series

[ovs-dev,v3,3/4] ovn-northd: Refactor parsing of *_force_snat_ip.

Message ID 20200917125121.19729.67492.stgit@dceara.remote.csb
State Changes Requested
Headers show
Series Drop packets destined to own IPs and refactor SNAT processing. | expand

Commit Message

Dumitru Ceara Sept. 17, 2020, 12:51 p.m. UTC 
Avoid reparsing the *_force_snat_ip addresses for every logical router port.
These addresses are defined once for the whole router.

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 lib/ovn-util.c      |    6 ++++
 lib/ovn-util.h      |    2 +
 northd/ovn-northd.c |   69 ++++++++++++++++++++++++---------------------------
 3 files changed, 40 insertions(+), 37 deletions(-)

Comments

Han Zhou Sept. 21, 2020, 7:01 a.m. UTC | #1 
On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> Avoid reparsing the *_force_snat_ip addresses for every logical router
port.
> These addresses are defined once for the whole router.
>

The commit message is a little misleading. Originally it wasn't reparsing
for every logical router port. It was per-router. This patch avoids one
extra parsing.

In addition, another minor comment is that the function name
"empty_lport_addresses" may be "lport_addresses_is_empty" to follow the
naming convention of a lot of xxx_is_empty() function names in OVN/OVS.

With these addressed:
Acked-by: Han Zhou <hzhou@ovn.org>

> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> ---
>  lib/ovn-util.c      |    6 ++++
>  lib/ovn-util.h      |    2 +
>  northd/ovn-northd.c |   69
++++++++++++++++++++++++---------------------------
>  3 files changed, 40 insertions(+), 37 deletions(-)
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index cdb5e18..a8cf6c9 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -320,6 +320,12 @@ extract_sbrec_binding_first_mac(const struct
sbrec_port_binding *binding,
>      return ret;
>  }
>
> +bool
> +empty_lport_addresses(struct lport_addresses *laddrs)
> +{
> +    return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
> +}
> +
>  void
>  destroy_lport_addresses(struct lport_addresses *laddrs)
>  {
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index d9aadcb..3343f28 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -74,7 +74,7 @@ bool extract_lrp_networks(const struct
nbrec_logical_router_port *,
>                            struct lport_addresses *);
>  bool extract_sbrec_binding_first_mac(const struct sbrec_port_binding
*binding,
>                                       struct eth_addr *ea);
> -
> +bool empty_lport_addresses(struct lport_addresses *);
>  void destroy_lport_addresses(struct lport_addresses *);
>
>  char *alloc_nat_zone_key(const struct uuid *key, const char *type);
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index f79ed99..43bd7b5 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -625,6 +625,8 @@ struct ovn_datapath {
>
>      /* SNAT IPs used by the router. */
>      struct sset snat_ips;
> +    struct lport_addresses dnat_force_snat_addrs;
> +    struct lport_addresses lb_force_snat_addrs;
>
>      struct ovn_port **localnet_ports;
>      size_t n_localnet_ports;
> @@ -670,32 +672,31 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry)
>  static void
>  init_nat_entries(struct ovn_datapath *od)
>  {
> -    struct lport_addresses snat_addrs;
> -
>      if (!od->nbr) {
>          return;
>      }
>
>      sset_init(&od->snat_ips);
> -    if (get_force_snat_ip(od, "dnat", &snat_addrs)) {
> -        if (snat_addrs.n_ipv4_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> +    if (get_force_snat_ip(od, "dnat", &od->dnat_force_snat_addrs)) {
> +        if (&od->dnat_force_snat_addrs.n_ipv4_addrs) {
> +            sset_add(&od->snat_ips,
> +                     od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s);
>          }
> -        if (snat_addrs.n_ipv6_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> +        if (&od->dnat_force_snat_addrs.n_ipv6_addrs) {
> +            sset_add(&od->snat_ips,
> +                     od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s);
>          }
> -        destroy_lport_addresses(&snat_addrs);
>      }
>
> -    memset(&snat_addrs, 0, sizeof(snat_addrs));
> -    if (get_force_snat_ip(od, "lb", &snat_addrs)) {
> -        if (snat_addrs.n_ipv4_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> +    if (get_force_snat_ip(od, "lb", &od->lb_force_snat_addrs)) {
> +        if (od->lb_force_snat_addrs.n_ipv4_addrs) {
> +            sset_add(&od->snat_ips,
> +                     od->lb_force_snat_addrs.ipv4_addrs[0].addr_s);
>          }
> -        if (snat_addrs.n_ipv6_addrs) {
> -            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> +        if (od->lb_force_snat_addrs.n_ipv6_addrs) {
> +            sset_add(&od->snat_ips,
> +                     od->lb_force_snat_addrs.ipv6_addrs[0].addr_s);
>          }
> -        destroy_lport_addresses(&snat_addrs);
>      }
>
>      if (!od->nbr->n_nat) {
> @@ -736,6 +737,9 @@ destroy_nat_entries(struct ovn_datapath *od)
>      }
>
>      sset_destroy(&od->snat_ips);
> +    destroy_lport_addresses(&od->dnat_force_snat_addrs);
> +    destroy_lport_addresses(&od->lb_force_snat_addrs);
> +
>      for (size_t i = 0; i < od->nbr->n_nat; i++) {
>          destroy_lport_addresses(&od->nat_entries[i].ext_addrs);
>      }
> @@ -9477,12 +9481,10 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
>
>          struct sset nat_entries = SSET_INITIALIZER(&nat_entries);
>
> -        struct lport_addresses dnat_force_snat_addrs;
> -        struct lport_addresses lb_force_snat_addrs;
> -        bool dnat_force_snat_ip = get_force_snat_ip(od, "dnat",
> -
 &dnat_force_snat_addrs);
> -        bool lb_force_snat_ip = get_force_snat_ip(od, "lb",
> -                                                  &lb_force_snat_addrs);
> +        bool dnat_force_snat_ip =
> +            !empty_lport_addresses(&od->dnat_force_snat_addrs);
> +        bool lb_force_snat_ip =
> +            !empty_lport_addresses(&od->lb_force_snat_addrs);
>
>          for (int i = 0; i < od->nbr->n_nat; i++) {
>              const struct nbrec_nat *nat;
> @@ -9982,23 +9984,25 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
>          /* Handle force SNAT options set in the gateway router. */
>          if (!od->l3dgw_port) {
>              if (dnat_force_snat_ip) {
> -                if (dnat_force_snat_addrs.n_ipv4_addrs) {
> +                if (od->dnat_force_snat_addrs.n_ipv4_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "4",
> -                        dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
"dnat");
> +                        od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
> +                        "dnat");
>                  }
> -                if (dnat_force_snat_addrs.n_ipv6_addrs) {
> +                if (od->dnat_force_snat_addrs.n_ipv6_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "6",
> -                        dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
"dnat");
> +                        od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
> +                        "dnat");
>                  }
>              }
>              if (lb_force_snat_ip) {
> -                if (lb_force_snat_addrs.n_ipv4_addrs) {
> +                if (od->lb_force_snat_addrs.n_ipv4_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "4",
> -                        lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb");
> +                        od->lb_force_snat_addrs.ipv4_addrs[0].addr_s,
"lb");
>                  }
> -                if (lb_force_snat_addrs.n_ipv6_addrs) {
> +                if (od->lb_force_snat_addrs.n_ipv6_addrs) {
>                      build_lrouter_force_snat_flows(lflows, od, "6",
> -                        lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
> +                        od->lb_force_snat_addrs.ipv6_addrs[0].addr_s,
"lb");
>                  }
>              }
>
> @@ -10015,13 +10019,6 @@ build_lrouter_flows(struct hmap *datapaths,
struct hmap *ports,
>                            "ip", "flags.loopback = 1; ct_dnat;");
>          }
>
> -        if (dnat_force_snat_ip) {
> -            destroy_lport_addresses(&dnat_force_snat_addrs);
> -        }
> -        if (lb_force_snat_ip) {
> -            destroy_lport_addresses(&lb_force_snat_addrs);
> -        }
> -
>          /* Load balancing and packet defrag are only valid on
>           * Gateway routers or router with gateway port. */
>          if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port) {
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Numan Siddique Sept. 22, 2020, 6:11 a.m. UTC | #2 
On Mon, Sep 21, 2020 at 12:32 PM Han Zhou <hzhou@ovn.org> wrote:

> On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> > Avoid reparsing the *_force_snat_ip addresses for every logical router
> port.
> > These addresses are defined once for the whole router.
> >
>
> The commit message is a little misleading. Originally it wasn't reparsing
> for every logical router port. It was per-router. This patch avoids one
> extra parsing.
>
> In addition, another minor comment is that the function name
> "empty_lport_addresses" may be "lport_addresses_is_empty" to follow the
> naming convention of a lot of xxx_is_empty() function names in OVN/OVS.
>
> With these addressed:
> Acked-by: Han Zhou <hzhou@ovn.org>
>


Hi Dumitru,

The compilation fails for clang with the below messages

*****

libtool: link: clang -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare
-Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
-Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition
-Wmissing-prototypes -Wmissing-field-initializers -Wthread-safety
-fno-strict-aliasing -Wswitch-bool -Wlogical-not-parentheses
-Wsizeof-array-argument -Wshift-negative-value -Qunused-arguments -Wshadow
-Wno-null-pointer-arithmetic -Warray-bounds-pointer-arithmetic -Werror
-Werror -g -O2 -o ic/ovn-ic ic/ovn-ic.o  lib/.libs/libovn.a
/home/nusiddiq/work/ovs_ovn/ovs/ovsdb/.libs/libovsdb.a
/home/nusiddiq/work/ovs_ovn/ovs/lib/.libs/libopenvswitch.a -lssl -lcrypto
-lcap-ng /home/nusiddiq/work/ovs_ovn/ovs/lib/.libs/libopenvswitchavx512.a
-lpthread -lrt -lm -lunbound
../northd/ovn-northd.c:681:40: error: address of
'od->dnat_force_snat_addrs.n_ipv4_addrs' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
        if (&od->dnat_force_snat_addrs.n_ipv4_addrs) {
        ~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../northd/ovn-northd.c:685:40: error: address of
'od->dnat_force_snat_addrs.n_ipv6_addrs' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
        if (&od->dnat_force_snat_addrs.n_ipv6_addrs) {
        ~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
*******

 I think there is no need for '&'.

Thanks
Numan


> > Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> > ---
> >  lib/ovn-util.c      |    6 ++++
> >  lib/ovn-util.h      |    2 +
> >  northd/ovn-northd.c |   69
> ++++++++++++++++++++++++---------------------------
> >  3 files changed, 40 insertions(+), 37 deletions(-)
> >
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index cdb5e18..a8cf6c9 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -320,6 +320,12 @@ extract_sbrec_binding_first_mac(const struct
> sbrec_port_binding *binding,
> >      return ret;
> >  }
> >
> > +bool
> > +empty_lport_addresses(struct lport_addresses *laddrs)
> > +{
> > +    return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
> > +}
> > +
> >  void
> >  destroy_lport_addresses(struct lport_addresses *laddrs)
> >  {
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index d9aadcb..3343f28 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -74,7 +74,7 @@ bool extract_lrp_networks(const struct
> nbrec_logical_router_port *,
> >                            struct lport_addresses *);
> >  bool extract_sbrec_binding_first_mac(const struct sbrec_port_binding
> *binding,
> >                                       struct eth_addr *ea);
> > -
> > +bool empty_lport_addresses(struct lport_addresses *);
> >  void destroy_lport_addresses(struct lport_addresses *);
> >
> >  char *alloc_nat_zone_key(const struct uuid *key, const char *type);
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index f79ed99..43bd7b5 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -625,6 +625,8 @@ struct ovn_datapath {
> >
> >      /* SNAT IPs used by the router. */
> >      struct sset snat_ips;
> > +    struct lport_addresses dnat_force_snat_addrs;
> > +    struct lport_addresses lb_force_snat_addrs;
> >
> >      struct ovn_port **localnet_ports;
> >      size_t n_localnet_ports;
> > @@ -670,32 +672,31 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry)
> >  static void
> >  init_nat_entries(struct ovn_datapath *od)
> >  {
> > -    struct lport_addresses snat_addrs;
> > -
> >      if (!od->nbr) {
> >          return;
> >      }
> >
> >      sset_init(&od->snat_ips);
> > -    if (get_force_snat_ip(od, "dnat", &snat_addrs)) {
> > -        if (snat_addrs.n_ipv4_addrs) {
> > -            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> > +    if (get_force_snat_ip(od, "dnat", &od->dnat_force_snat_addrs)) {
> > +        if (&od->dnat_force_snat_addrs.n_ipv4_addrs) {
> > +            sset_add(&od->snat_ips,
> > +                     od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s);
> >          }
> > -        if (snat_addrs.n_ipv6_addrs) {
> > -            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> > +        if (&od->dnat_force_snat_addrs.n_ipv6_addrs) {
> > +            sset_add(&od->snat_ips,
> > +                     od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s);
> >          }
> > -        destroy_lport_addresses(&snat_addrs);
> >      }
> >
> > -    memset(&snat_addrs, 0, sizeof(snat_addrs));
> > -    if (get_force_snat_ip(od, "lb", &snat_addrs)) {
> > -        if (snat_addrs.n_ipv4_addrs) {
> > -            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
> > +    if (get_force_snat_ip(od, "lb", &od->lb_force_snat_addrs)) {
> > +        if (od->lb_force_snat_addrs.n_ipv4_addrs) {
> > +            sset_add(&od->snat_ips,
> > +                     od->lb_force_snat_addrs.ipv4_addrs[0].addr_s);
> >          }
> > -        if (snat_addrs.n_ipv6_addrs) {
> > -            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
> > +        if (od->lb_force_snat_addrs.n_ipv6_addrs) {
> > +            sset_add(&od->snat_ips,
> > +                     od->lb_force_snat_addrs.ipv6_addrs[0].addr_s);
> >          }
> > -        destroy_lport_addresses(&snat_addrs);
> >      }
> >
> >      if (!od->nbr->n_nat) {
> > @@ -736,6 +737,9 @@ destroy_nat_entries(struct ovn_datapath *od)
> >      }
> >
> >      sset_destroy(&od->snat_ips);
> > +    destroy_lport_addresses(&od->dnat_force_snat_addrs);
> > +    destroy_lport_addresses(&od->lb_force_snat_addrs);
> > +
> >      for (size_t i = 0; i < od->nbr->n_nat; i++) {
> >          destroy_lport_addresses(&od->nat_entries[i].ext_addrs);
> >      }
> > @@ -9477,12 +9481,10 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> >
> >          struct sset nat_entries = SSET_INITIALIZER(&nat_entries);
> >
> > -        struct lport_addresses dnat_force_snat_addrs;
> > -        struct lport_addresses lb_force_snat_addrs;
> > -        bool dnat_force_snat_ip = get_force_snat_ip(od, "dnat",
> > -
>  &dnat_force_snat_addrs);
> > -        bool lb_force_snat_ip = get_force_snat_ip(od, "lb",
> > -                                                  &lb_force_snat_addrs);
> > +        bool dnat_force_snat_ip =
> > +            !empty_lport_addresses(&od->dnat_force_snat_addrs);
> > +        bool lb_force_snat_ip =
> > +            !empty_lport_addresses(&od->lb_force_snat_addrs);
> >
> >          for (int i = 0; i < od->nbr->n_nat; i++) {
> >              const struct nbrec_nat *nat;
> > @@ -9982,23 +9984,25 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> >          /* Handle force SNAT options set in the gateway router. */
> >          if (!od->l3dgw_port) {
> >              if (dnat_force_snat_ip) {
> > -                if (dnat_force_snat_addrs.n_ipv4_addrs) {
> > +                if (od->dnat_force_snat_addrs.n_ipv4_addrs) {

>                      build_lrouter_force_snat_flows(lflows, od, "4",
> > -                        dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
> "dnat");
> > +                        od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
> > +                        "dnat");
> >                  }
> > -                if (dnat_force_snat_addrs.n_ipv6_addrs) {
> > +                if (od->dnat_force_snat_addrs.n_ipv6_addrs) {
> >                      build_lrouter_force_snat_flows(lflows, od, "6",
> > -                        dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
> "dnat");
> > +                        od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
> > +                        "dnat");
> >                  }
> >              }
> >              if (lb_force_snat_ip) {
> > -                if (lb_force_snat_addrs.n_ipv4_addrs) {
> > +                if (od->lb_force_snat_addrs.n_ipv4_addrs) {
> >                      build_lrouter_force_snat_flows(lflows, od, "4",
> > -                        lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb");
> > +                        od->lb_force_snat_addrs.ipv4_addrs[0].addr_s,
> "lb");
> >                  }
> > -                if (lb_force_snat_addrs.n_ipv6_addrs) {
> > +                if (od->lb_force_snat_addrs.n_ipv6_addrs) {
> >                      build_lrouter_force_snat_flows(lflows, od, "6",
> > -                        lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
> > +                        od->lb_force_snat_addrs.ipv6_addrs[0].addr_s,
> "lb");
> >                  }
> >              }
> >
> > @@ -10015,13 +10019,6 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> >                            "ip", "flags.loopback = 1; ct_dnat;");
> >          }
> >
> > -        if (dnat_force_snat_ip) {
> > -            destroy_lport_addresses(&dnat_force_snat_addrs);
> > -        }
> > -        if (lb_force_snat_ip) {
> > -            destroy_lport_addresses(&lb_force_snat_addrs);
> > -        }
> > -
> >          /* Load balancing and packet defrag are only valid on
> >           * Gateway routers or router with gateway port. */
> >          if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port)
> {
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Dumitru Ceara Sept. 28, 2020, 7:53 a.m. UTC | #3 
On 9/22/20 8:11 AM, Numan Siddique wrote:
> 
> 
> On Mon, Sep 21, 2020 at 12:32 PM Han Zhou <hzhou@ovn.org
> <mailto:hzhou@ovn.org>> wrote:
> 
>     On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara@redhat.com
>     <mailto:dceara@redhat.com>> wrote:
>     >
>     > Avoid reparsing the *_force_snat_ip addresses for every logical router
>     port.
>     > These addresses are defined once for the whole router.
>     >
> 
>     The commit message is a little misleading. Originally it wasn't
>     reparsing
>     for every logical router port. It was per-router. This patch avoids one
>     extra parsing.
> 
>     In addition, another minor comment is that the function name
>     "empty_lport_addresses" may be "lport_addresses_is_empty" to follow the
>     naming convention of a lot of xxx_is_empty() function names in OVN/OVS.
> 
>     With these addressed:
>     Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>>
> 
> 
> 
> Hi Dumitru,
> 
> The compilation fails for clang with the below messages
> 
> *****
> 
> libtool: link: clang -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare
> -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
> -Wthread-safety -fno-strict-aliasing -Wswitch-bool
> -Wlogical-not-parentheses -Wsizeof-array-argument -Wshift-negative-value
> -Qunused-arguments -Wshadow -Wno-null-pointer-arithmetic
> -Warray-bounds-pointer-arithmetic -Werror -Werror -g -O2 -o ic/ovn-ic
> ic/ovn-ic.o  lib/.libs/libovn.a
> /home/nusiddiq/work/ovs_ovn/ovs/ovsdb/.libs/libovsdb.a
> /home/nusiddiq/work/ovs_ovn/ovs/lib/.libs/libopenvswitch.a -lssl
> -lcrypto -lcap-ng
> /home/nusiddiq/work/ovs_ovn/ovs/lib/.libs/libopenvswitchavx512.a
> -lpthread -lrt -lm -lunbound
> ../northd/ovn-northd.c:681:40: error: address of
> 'od->dnat_force_snat_addrs.n_ipv4_addrs' will always evaluate to 'true'
> [-Werror,-Wpointer-bool-conversion]
>         if (&od->dnat_force_snat_addrs.n_ipv4_addrs) {
>         ~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> ../northd/ovn-northd.c:685:40: error: address of
> 'od->dnat_force_snat_addrs.n_ipv6_addrs' will always evaluate to 'true'
> [-Werror,-Wpointer-bool-conversion]
>         if (&od->dnat_force_snat_addrs.n_ipv6_addrs) {
>         ~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> *******
> 
>  I think there is no need for '&'.
> 
> Thanks
> Numan
> 

Hi Numan,

You're right, this is a very nasty copy/paste error. I'll fix it in a
new revision. I'll also try to add a test to spot this kind of issues
earlier.

Regards,
Dumitru
Dumitru Ceara Sept. 28, 2020, 7:55 a.m. UTC | #4 
On 9/21/20 9:01 AM, Han Zhou wrote:
> 
> 
> On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara@redhat.com
> <mailto:dceara@redhat.com>> wrote:
>>
>> Avoid reparsing the *_force_snat_ip addresses for every logical router
> port.
>> These addresses are defined once for the whole router.
>>
> 
> The commit message is a little misleading. Originally it wasn't
> reparsing for every logical router port. It was per-router. This patch
> avoids one extra parsing.
> 
> In addition, another minor comment is that the function name
> "empty_lport_addresses" may be "lport_addresses_is_empty" to follow the
> naming convention of a lot of xxx_is_empty() function names in OVN/OVS.
> 
> With these addressed:
> Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>>
> 

Thanks Han for the review! I'll address your comments and fix the issue
Numan spotted and I'll respin this patch.

Regards,
Dumitru
diff mbox series

Patch

diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index cdb5e18..a8cf6c9 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -320,6 +320,12 @@  extract_sbrec_binding_first_mac(const struct sbrec_port_binding *binding,
     return ret;
 }
 
+bool
+empty_lport_addresses(struct lport_addresses *laddrs)
+{
+    return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
+}
+
 void
 destroy_lport_addresses(struct lport_addresses *laddrs)
 {
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index d9aadcb..3343f28 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -74,7 +74,7 @@  bool extract_lrp_networks(const struct nbrec_logical_router_port *,
                           struct lport_addresses *);
 bool extract_sbrec_binding_first_mac(const struct sbrec_port_binding *binding,
                                      struct eth_addr *ea);
-
+bool empty_lport_addresses(struct lport_addresses *);
 void destroy_lport_addresses(struct lport_addresses *);
 
 char *alloc_nat_zone_key(const struct uuid *key, const char *type);
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index f79ed99..43bd7b5 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -625,6 +625,8 @@  struct ovn_datapath {
 
     /* SNAT IPs used by the router. */
     struct sset snat_ips;
+    struct lport_addresses dnat_force_snat_addrs;
+    struct lport_addresses lb_force_snat_addrs;
 
     struct ovn_port **localnet_ports;
     size_t n_localnet_ports;
@@ -670,32 +672,31 @@  nat_entry_is_v6(const struct ovn_nat *nat_entry)
 static void
 init_nat_entries(struct ovn_datapath *od)
 {
-    struct lport_addresses snat_addrs;
-
     if (!od->nbr) {
         return;
     }
 
     sset_init(&od->snat_ips);
-    if (get_force_snat_ip(od, "dnat", &snat_addrs)) {
-        if (snat_addrs.n_ipv4_addrs) {
-            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
+    if (get_force_snat_ip(od, "dnat", &od->dnat_force_snat_addrs)) {
+        if (&od->dnat_force_snat_addrs.n_ipv4_addrs) {
+            sset_add(&od->snat_ips,
+                     od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s);
         }
-        if (snat_addrs.n_ipv6_addrs) {
-            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
+        if (&od->dnat_force_snat_addrs.n_ipv6_addrs) {
+            sset_add(&od->snat_ips,
+                     od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s);
         }
-        destroy_lport_addresses(&snat_addrs);
     }
 
-    memset(&snat_addrs, 0, sizeof(snat_addrs));
-    if (get_force_snat_ip(od, "lb", &snat_addrs)) {
-        if (snat_addrs.n_ipv4_addrs) {
-            sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s);
+    if (get_force_snat_ip(od, "lb", &od->lb_force_snat_addrs)) {
+        if (od->lb_force_snat_addrs.n_ipv4_addrs) {
+            sset_add(&od->snat_ips,
+                     od->lb_force_snat_addrs.ipv4_addrs[0].addr_s);
         }
-        if (snat_addrs.n_ipv6_addrs) {
-            sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s);
+        if (od->lb_force_snat_addrs.n_ipv6_addrs) {
+            sset_add(&od->snat_ips,
+                     od->lb_force_snat_addrs.ipv6_addrs[0].addr_s);
         }
-        destroy_lport_addresses(&snat_addrs);
     }
 
     if (!od->nbr->n_nat) {
@@ -736,6 +737,9 @@  destroy_nat_entries(struct ovn_datapath *od)
     }
 
     sset_destroy(&od->snat_ips);
+    destroy_lport_addresses(&od->dnat_force_snat_addrs);
+    destroy_lport_addresses(&od->lb_force_snat_addrs);
+
     for (size_t i = 0; i < od->nbr->n_nat; i++) {
         destroy_lport_addresses(&od->nat_entries[i].ext_addrs);
     }
@@ -9477,12 +9481,10 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
 
         struct sset nat_entries = SSET_INITIALIZER(&nat_entries);
 
-        struct lport_addresses dnat_force_snat_addrs;
-        struct lport_addresses lb_force_snat_addrs;
-        bool dnat_force_snat_ip = get_force_snat_ip(od, "dnat",
-                                                    &dnat_force_snat_addrs);
-        bool lb_force_snat_ip = get_force_snat_ip(od, "lb",
-                                                  &lb_force_snat_addrs);
+        bool dnat_force_snat_ip =
+            !empty_lport_addresses(&od->dnat_force_snat_addrs);
+        bool lb_force_snat_ip =
+            !empty_lport_addresses(&od->lb_force_snat_addrs);
 
         for (int i = 0; i < od->nbr->n_nat; i++) {
             const struct nbrec_nat *nat;
@@ -9982,23 +9984,25 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
         /* Handle force SNAT options set in the gateway router. */
         if (!od->l3dgw_port) {
             if (dnat_force_snat_ip) {
-                if (dnat_force_snat_addrs.n_ipv4_addrs) {
+                if (od->dnat_force_snat_addrs.n_ipv4_addrs) {
                     build_lrouter_force_snat_flows(lflows, od, "4",
-                        dnat_force_snat_addrs.ipv4_addrs[0].addr_s, "dnat");
+                        od->dnat_force_snat_addrs.ipv4_addrs[0].addr_s,
+                        "dnat");
                 }
-                if (dnat_force_snat_addrs.n_ipv6_addrs) {
+                if (od->dnat_force_snat_addrs.n_ipv6_addrs) {
                     build_lrouter_force_snat_flows(lflows, od, "6",
-                        dnat_force_snat_addrs.ipv6_addrs[0].addr_s, "dnat");
+                        od->dnat_force_snat_addrs.ipv6_addrs[0].addr_s,
+                        "dnat");
                 }
             }
             if (lb_force_snat_ip) {
-                if (lb_force_snat_addrs.n_ipv4_addrs) {
+                if (od->lb_force_snat_addrs.n_ipv4_addrs) {
                     build_lrouter_force_snat_flows(lflows, od, "4",
-                        lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb");
+                        od->lb_force_snat_addrs.ipv4_addrs[0].addr_s, "lb");
                 }
-                if (lb_force_snat_addrs.n_ipv6_addrs) {
+                if (od->lb_force_snat_addrs.n_ipv6_addrs) {
                     build_lrouter_force_snat_flows(lflows, od, "6",
-                        lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
+                        od->lb_force_snat_addrs.ipv6_addrs[0].addr_s, "lb");
                 }
             }
 
@@ -10015,13 +10019,6 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                           "ip", "flags.loopback = 1; ct_dnat;");
         }
 
-        if (dnat_force_snat_ip) {
-            destroy_lport_addresses(&dnat_force_snat_addrs);
-        }
-        if (lb_force_snat_ip) {
-            destroy_lport_addresses(&lb_force_snat_addrs);
-        }
-
         /* Load balancing and packet defrag are only valid on
          * Gateway routers or router with gateway port. */
         if (!smap_get(&od->nbr->options, "chassis") && !od->l3dgw_port) {