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); } _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev