On Mon, Sep 21, 2020 at 12:32 PM Han Zhou <hz...@ovn.org> wrote: > On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dce...@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 <hz...@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 <dce...@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 > > 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 > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev