[ovs-dev,v2] ovn-northd: Combine two NAT loops into one.
diff mbox

Message ID 1468408836-20417-1-git-send-email-guru@ovn.org
State Accepted
Headers show

Commit Message

Guru Shetty July 13, 2016, 11:20 a.m. UTC
Signed-off-by: Gurucharan Shetty <guru@ovn.org>
---
 ovn/northd/ovn-northd.c | 47 ++++++++++++++---------------------------------
 1 file changed, 14 insertions(+), 33 deletions(-)

Comments

Ben Pfaff July 26, 2016, 5:19 p.m. UTC | #1
On Wed, Jul 13, 2016 at 04:20:36AM -0700, Gurucharan Shetty wrote:
> Signed-off-by: Gurucharan Shetty <guru@ovn.org>
> ---
>  ovn/northd/ovn-northd.c | 47 ++++++++++++++---------------------------------
>  1 file changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index b1c2c6c..52e3229 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2330,27 +2330,30 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                            ds_cstr(&match), ds_cstr(&actions));
>          }
>  
> -        /* ARP handling for external IP addresses.
> -         *
> -         * DNAT IP addresses are external IP addresses that need ARP
> -         * handling. */
> +        ovs_be32 *nat_ips = xmalloc(sizeof *nat_ips * op->od->nbr->n_nat);
> +        size_t n_snat_ips = 0;

The code might be a little clearer with s/nat_ips/snat_ips/, to match
the name of n_snat_ips.

Acked-by: Ben Pfaff <blp@ovn.org>
Guru Shetty July 26, 2016, 5:57 p.m. UTC | #2
On 26 July 2016 at 10:19, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Jul 13, 2016 at 04:20:36AM -0700, Gurucharan Shetty wrote:
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
> > ---
> >  ovn/northd/ovn-northd.c | 47
> ++++++++++++++---------------------------------
> >  1 file changed, 14 insertions(+), 33 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index b1c2c6c..52e3229 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -2330,27 +2330,30 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> >                            ds_cstr(&match), ds_cstr(&actions));
> >          }
> >
> > -        /* ARP handling for external IP addresses.
> > -         *
> > -         * DNAT IP addresses are external IP addresses that need ARP
> > -         * handling. */
> > +        ovs_be32 *nat_ips = xmalloc(sizeof *nat_ips *
> op->od->nbr->n_nat);
> > +        size_t n_snat_ips = 0;
>
> The code might be a little clearer with s/nat_ips/snat_ips/, to match
> the name of n_snat_ips.
>

Thanks, I made the change as suggested and applied this.


>
> Acked-by: Ben Pfaff <blp@ovn.org>
>

Patch
diff mbox

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index b1c2c6c..52e3229 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2330,27 +2330,30 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                           ds_cstr(&match), ds_cstr(&actions));
         }
 
-        /* ARP handling for external IP addresses.
-         *
-         * DNAT IP addresses are external IP addresses that need ARP
-         * handling. */
+        ovs_be32 *nat_ips = xmalloc(sizeof *nat_ips * op->od->nbr->n_nat);
+        size_t n_snat_ips = 0;
         for (int i = 0; i < op->od->nbr->n_nat; i++) {
             const struct nbrec_nat *nat;
 
             nat = op->od->nbr->nat[i];
 
-            if(!strcmp(nat->type, "snat")) {
-                continue;
-            }
-
             ovs_be32 ip;
             if (!ip_parse(nat->external_ip, &ip) || !ip) {
                 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad ip address %s in dnat configuration "
+                VLOG_WARN_RL(&rl, "bad ip address %s in nat configuration "
                              "for router %s", nat->external_ip, op->key);
                 continue;
             }
 
+            if (!strcmp(nat->type, "snat")) {
+                nat_ips[n_snat_ips++] = ip;
+                continue;
+            }
+
+            /* ARP handling for external IP addresses.
+             *
+             * DNAT IP addresses are external IP addresses that need ARP
+             * handling. */
             ds_clear(&match);
             ds_put_format(&match,
                           "inport == %s && arp.tpa == "IP_FMT" && arp.op == 1",
@@ -2376,34 +2379,12 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                           ds_cstr(&match), ds_cstr(&actions));
         }
 
-        /* Drop IP traffic to this router, unless the router ip is used as
-         * SNAT ip. */
-        ovs_be32 *nat_ips = xmalloc(sizeof *nat_ips * op->od->nbr->n_nat);
-        size_t n_nat_ips = 0;
-        for (int i = 0; i < op->od->nbr->n_nat; i++) {
-            const struct nbrec_nat *nat;
-            ovs_be32 ip;
-
-            nat = op->od->nbr->nat[i];
-            if (strcmp(nat->type, "snat")) {
-                continue;
-            }
-
-            if (!ip_parse(nat->external_ip, &ip) || !ip) {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "bad ip address %s in snat configuration "
-                         "for router %s", nat->external_ip, op->key);
-                continue;
-            }
-
-            nat_ips[n_nat_ips++] = ip;
-        }
-
         ds_clear(&match);
         ds_put_cstr(&match, "ip4.dst == {");
         bool has_drop_ips = false;
         for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
-            for (int j = 0; j < n_nat_ips; j++) {
+            for (int j = 0; j < n_snat_ips; j++) {
+                /* Packets to SNAT IPs should not be dropped. */
                 if (op->lrp_networks.ipv4_addrs[i].addr == nat_ips[j]) {
                     continue;
                 }