[ovs-dev] [PATCH v3 ovn 3/4] ovn-northd: Refactor parsing of *_force_snat_ip.
Numan Siddique
numans at ovn.org
Tue Sep 22 06:11:43 UTC 2020
On Mon, Sep 21, 2020 at 12:32 PM Han Zhou <hzhou at ovn.org> wrote:
> On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara at 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 at 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 at 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 at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list