[ovs-dev] [mirror 13/13] ofproto-dpif: Get rid of "struct dst".

[Ben Pfaff]

On Wed, Nov 16, 2011 at 11:51:25PM -0800, Justin Pettit wrote:
> On Oct 26, 2011, at 10:09 AM, Ben Pfaff wrote:
> 
> > static bool
> > is_admissible(struct ofproto_dpif *ofproto, const struct flow *flow,
> > -              bool have_packet,
> > -              tag_type *tags, int *vlanp, struct ofbundle **in_bundlep)
> > +              struct ofport_dpif *in_port, uint16_t vlan, bool warn,
> > +              tag_type *tags)
> 
> The comment describing this function needs to be updated, since the
> argument have changed substantially.

Good point.  Updated to:

/* Determines whether packets in 'flow' within 'ofproto' should be forwarded or
 * dropped.  Returns true if they may be forwarded, false if they should be
 * dropped.
 *
 * 'in_port' must be the ofport_dpif that corresponds to flow->in_port.
 * 'in_port' must be part of a bundle (e.g. in_port->bundle must be nonnull).
 *
 * 'vlan' must be the VLAN that corresponds to flow->vlan_tci on 'in_port', as
 * returned by input_vid_to_vlan().  It must be a valid VLAN for 'in_port', as
 * checked by input_vid_is_valid().
 *
 * May also add tags to '*tags', although the current implementation only does
 * so in one special case.
 */

I dropped the "warn" parameter.

> > +    /* Check other admissibility requirements. */
> > +    if (!is_admissible(ctx->ofproto, &ctx->flow, in_port, vlan,
> > +                       ctx->packet != NULL, &ctx->tags)) {
> > +        output_mirrors(ctx, vlan, in_bundle, 0);
> 
> Do we always want to mirror packets that were determined to be not
> admissible?  For example, should we be mirroring frames that came in
> on a destination mirror bundle?

Another good point.  We shouldn't mirror frames that come in on a
destination mirror bundle.  I moved that check into xlate_normal()
above the "Check VLAN." block.

It seems reasonable to mirror in the other cases to me, so I left them
alone.

> > +    if (in_bundle != out_bundle) {
> > +        compose_dsts(ctx, vlan, in_bundle, out_bundle);
> 
> I assume you want to store the result in "dst_mirrors", since it's
> otherwise never set and output mirrors will never be used.

Another good point.  Thanks, I added "dst_mirrors =".

This series had a lot of damage from merge conflicts etc.  Do you want
to look at any of it again?

Here's a incremental that omits the merge conflict resolution:

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 9f25534..86723e1 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4736,26 +4736,19 @@ lookup_input_bundle(struct ofproto_dpif *ofproto, uint16_t in_port, bool warn)
  * dropped.  Returns true if they may be forwarded, false if they should be
  * dropped.
  *
- * If 'have_packet' is true, it indicates that the caller is processing a
- * received packet.  If 'have_packet' is false, then the caller is just
- * revalidating an existing flow because configuration has changed.  Either
- * way, 'have_packet' only affects logging (there is no point in logging errors
- * during revalidation).
+ * 'in_port' must be the ofport_dpif that corresponds to flow->in_port.
+ * 'in_port' must be part of a bundle (e.g. in_port->bundle must be nonnull).
  *
- * Sets '*in_bundlep' to the input bundle.  This will be a null pointer if
- * flow->in_port does not designate a known input port (in which case
- * is_admissible() returns false).
- *
- * When returning true, sets '*vlanp' to the effective VLAN of the input
- * packet, as returned by input_vid_to_vlan().
+ * 'vlan' must be the VLAN that corresponds to flow->vlan_tci on 'in_port', as
+ * returned by input_vid_to_vlan().  It must be a valid VLAN for 'in_port', as
+ * checked by input_vid_is_valid().
  *
  * May also add tags to '*tags', although the current implementation only does
  * so in one special case.
  */
 static bool
 is_admissible(struct ofproto_dpif *ofproto, const struct flow *flow,
-              struct ofport_dpif *in_port, uint16_t vlan, bool warn,
-              tag_type *tags)
+              struct ofport_dpif *in_port, uint16_t vlan, tag_type *tags)
 {
     struct ofbundle *in_bundle = in_port->bundle;
 
@@ -4765,17 +4758,6 @@ is_admissible(struct ofproto_dpif *ofproto, const struct flow *flow,
         return false;
     }
 
-    /* Drop frames on bundles reserved for mirroring. */
-    if (in_bundle->mirror_out) {
-        if (warn) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-            VLOG_WARN_RL(&rl, "bridge %s: dropping packet received on port "
-                         "%s, which is reserved exclusively for mirroring",
-                         ofproto->up.name, in_bundle->name);
-        }
-        return false;
-    }
-
     if (in_bundle->bond) {
         struct mac_entry *mac;
 
@@ -4827,10 +4809,23 @@ xlate_normal(struct action_xlate_ctx *ctx)
     /* Drop malformed frames. */
     if (ctx->flow.dl_type == htons(ETH_TYPE_VLAN) &&
         !(ctx->flow.vlan_tci & htons(VLAN_CFI))) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-        VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "
-                     "VLAN tag received on port %s",
-                     ctx->ofproto->up.name, in_bundle->name);
+        if (ctx->packet != NULL) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+            VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "
+                         "VLAN tag received on port %s",
+                         ctx->ofproto->up.name, in_bundle->name);
+        }
+        return;
+    }
+
+    /* Drop frames on bundles reserved for mirroring. */
+    if (in_bundle->mirror_out) {
+        if (ctx->packet != NULL) {
+            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+            VLOG_WARN_RL(&rl, "bridge %s: dropping packet received on port "
+                         "%s, which is reserved exclusively for mirroring",
+                         ctx->ofproto->up.name, in_bundle->name);
+        }
         return;
     }
 
@@ -4842,8 +4837,7 @@ xlate_normal(struct action_xlate_ctx *ctx)
     vlan = input_vid_to_vlan(in_bundle, vid);
 
     /* Check other admissibility requirements. */
-    if (!is_admissible(ctx->ofproto, &ctx->flow, in_port, vlan,
-                       ctx->packet != NULL, &ctx->tags)) {
+    if (!is_admissible(ctx->ofproto, &ctx->flow, in_port, vlan, &ctx->tags)) {
         output_mirrors(ctx, vlan, in_bundle, 0);
         return;
     }
@@ -4872,7 +4866,7 @@ xlate_normal(struct action_xlate_ctx *ctx)
 
     /* Don't send packets out their input bundles. */
     if (in_bundle != out_bundle) {
-        compose_dsts(ctx, vlan, in_bundle, out_bundle);
+        dst_mirrors = compose_dsts(ctx, vlan, in_bundle, out_bundle);
     }
     output_mirrors(ctx, vlan, in_bundle, dst_mirrors);
 }