[ovs-dev] [xlate v1 12/18] connmgr: Remove connmgr_must_output_local().

Ben Pfaff blp at nicira.com
Wed Jun 26 21:45:32 UTC 2013


On Wed, Jun 26, 2013 at 02:13:03PM -0700, Ben Pfaff wrote:
> On Mon, Jun 24, 2013 at 06:59:26PM -0700, Ethan Jackson wrote:
> > connmgr_must_output_local() requires a 'struct connmgr' handle,
> > when in principle, it should simply be enough to know whether or
> > not in_band is enabled.  Breaking this up will allow
> > ofproto-dpif-xlate to disentangle itself from ofproto-dpif in future
> > patches.
> > 
> > Signed-off-by: Ethan Jackson <ethan at nicira.com>
> 
> This is an improvement, thanks.
> 
> Acked-by: Ben Pfaff <blp at nicira.com>

Here's a patch you might consider insert in the series after that one.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Wed, 26 Jun 2013 14:44:39 -0700
Subject: [PATCH] ofproto-dpif: Refactor checking for in-band special case.

The comments on in_band_rule_check() were more or less wrong (the return
value was no longer used to determine whether a flow could be set up).
This commit fixes the comments and refactors the interface to make better
sense in the current context.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/in-band.c            |   29 +++++++----------------------
 ofproto/in-band.h            |    5 ++---
 ofproto/ofproto-dpif-xlate.c |   23 ++++++++++++++++++-----
 3 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index 0746bab..03d4661 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -222,31 +222,16 @@ refresh_local(struct in_band *ib)
     return true;
 }
 
-/* Returns true if the rule that would match 'flow' with 'actions' is
- * allowed to be set up in the datapath. */
+/* Returns true if packets in 'flow' should be directed to the local port.
+ * (This keeps the flow table from preventing DHCP replies from being seen by
+ * the local port.) */
 bool
-in_band_rule_check(const struct flow *flow, odp_port_t local_odp_port,
-                   const struct nlattr *actions, size_t actions_len)
+in_band_must_output_to_local_port(const struct flow *flow)
 {
-    /* Don't allow flows that would prevent DHCP replies from being seen
-     * by the local port. */
-    if (flow->dl_type == htons(ETH_TYPE_IP)
+    return (flow->dl_type == htons(ETH_TYPE_IP)
             && flow->nw_proto == IPPROTO_UDP
             && flow->tp_src == htons(DHCP_SERVER_PORT)
-            && flow->tp_dst == htons(DHCP_CLIENT_PORT)) {
-        const struct nlattr *a;
-        unsigned int left;
-
-        NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
-            if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT
-                && nl_attr_get_odp_port(a) == local_odp_port) {
-                return true;
-            }
-        }
-        return false;
-    }
-
-    return true;
+            && flow->tp_dst == htons(DHCP_CLIENT_PORT));
 }
 
 static void
diff --git a/ofproto/in-band.h b/ofproto/in-band.h
index 5449312..ad16dc2 100644
--- a/ofproto/in-band.h
+++ b/ofproto/in-band.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -40,7 +40,6 @@ void in_band_set_remotes(struct in_band *,
 bool in_band_run(struct in_band *);
 void in_band_wait(struct in_band *);
 
-bool in_band_rule_check(const struct flow *, odp_port_t local_odp_port,
-                        const struct nlattr *odp_actions, size_t actions_len);
+bool in_band_must_output_to_local_port(const struct flow *);
 
 #endif /* in-band.h */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index cb429ef..a542995 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1831,6 +1831,22 @@ xlate_out_copy(struct xlate_out *dst, const struct xlate_out *src)
                src->odp_actions.size);
 }
 
+static bool
+actions_output_to_local_port(const struct xlate_ctx *ctx)
+{
+    odp_port_t local_odp_port = ofp_port_to_odp_port(ctx->ofproto, OFPP_LOCAL);
+    const struct nlattr *a;
+    unsigned int left;
+
+    NL_ATTR_FOR_EACH_UNSAFE (a, left, ctx->xout->odp_actions.data,
+                             ctx->xout->odp_actions.size) {
+        if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT
+            && nl_attr_get_odp_port(a) == local_odp_port) {
+            return true;
+        }
+    }
+    return false;
+}
 
 /* Translates the 'ofpacts_len' bytes of "struct ofpacts" starting at 'ofpacts'
  * into datapath actions in 'odp_actions', using 'ctx'. */
@@ -1963,7 +1979,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     } else {
         static struct vlog_rate_limit trace_rl = VLOG_RATE_LIMIT_INIT(1, 1);
         size_t sample_actions_len;
-        odp_port_t local_odp_port;
 
         if (flow->in_port.ofp_port
             != vsp_realdev_to_vlandev(ctx.ofproto, flow->in_port.ofp_port,
@@ -2001,11 +2016,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             }
         }
 
-        local_odp_port = ofp_port_to_odp_port(ctx.ofproto, OFPP_LOCAL);
         if (connmgr_has_in_band(ctx.ofproto->up.connmgr)
-            && !in_band_rule_check(flow, local_odp_port,
-                                   ctx.xout->odp_actions.data,
-                                   ctx.xout->odp_actions.size)) {
+            && in_band_must_output_to_local_port(flow)
+            && !actions_output_to_local_port(&ctx)) {
             compose_output_action(&ctx, OFPP_LOCAL);
         }
         if (mirror_ofproto_has_mirrors(ctx.ofproto)) {
-- 
1.7.2.5




More information about the dev mailing list