Thanks. QA gave this an OK so I pushed it to branch-1.2.
On Fri, Oct 21, 2011 at 07:11:34PM -0700, Ethan Jackson wrote: > Looks good. > > Ethan > > > On Thu, Oct 13, 2011 at 11:10, Ben Pfaff <[email protected]> wrote: > > This is a nontrivial cross-port of commit 823518f1f "ofproto-dpif: Fix VLAN > > and other field handling in OFPP_NORMAL" from "master" to "branch-1.2". > > > > compose_actions(), which is part of the OFPP_NORMAL implementation, had > > multiple flaws that this commit corrects. > > > > First, it did not commit changes made to the flow by actions preceding the > > output to OFPP_NORMAL. ?This means that, for example, if an OpenFlow action > > to modify an L2 or L3 header preceded the output to OFPP_NORMAL, then > > OFPP_NORMAL would output its packets without those changes. > > > > Second, it did not update the action_xlate_ctx's notion of the VLAN that > > was currently set, in the cases where it modified the current VLAN. ?This > > means that actions following the output to OFPP_NORMAL could output to > > unexpected VLANs. > > --- > > I have not yet tested this, beyond the unit tests. ?I will ask our > > QA department to run their tests on it before I push it to branch-1.2. > > > > ?ofproto/ofproto-dpif.c | ? 47 > > ++++++++++++++++++++++++++++++----------------- > > ?1 files changed, 30 insertions(+), 17 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 2d0b83c..d5e276c 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -2801,6 +2801,23 @@ static void do_xlate_actions(const union ofp_action > > *in, size_t n_in, > > ?static void xlate_normal(struct action_xlate_ctx *); > > > > ?static void > > +commit_vlan_tci(struct action_xlate_ctx *ctx, ovs_be16 vlan_tci) > > +{ > > + ? ?struct flow *base = &ctx->base_flow; > > + ? ?struct ofpbuf *odp_actions = ctx->odp_actions; > > + > > + ? ?if (base->vlan_tci != vlan_tci) { > > + ? ? ? ?if (!(vlan_tci & htons(VLAN_CFI))) { > > + ? ? ? ? ? ?nl_msg_put_flag(odp_actions, ODP_ACTION_ATTR_STRIP_VLAN); > > + ? ? ? ?} else { > > + ? ? ? ? ? ?nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_DL_TCI, > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ?vlan_tci & ~htons(VLAN_CFI)); > > + ? ? ? ?} > > + ? ? ? ?base->vlan_tci = vlan_tci; > > + ? ?} > > +} > > + > > +static void > > ?commit_odp_actions(struct action_xlate_ctx *ctx) > > ?{ > > ? ? const struct flow *flow = &ctx->flow; > > @@ -2827,15 +2844,7 @@ commit_odp_actions(struct action_xlate_ctx *ctx) > > ? ? ? ? base->nw_tos = flow->nw_tos; > > ? ? } > > > > - ? ?if (base->vlan_tci != flow->vlan_tci) { > > - ? ? ? ?if (!(flow->vlan_tci & htons(VLAN_CFI))) { > > - ? ? ? ? ? ?nl_msg_put_flag(odp_actions, ODP_ACTION_ATTR_STRIP_VLAN); > > - ? ? ? ?} else { > > - ? ? ? ? ? ?nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_DL_TCI, > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ?flow->vlan_tci & ~htons(VLAN_CFI)); > > - ? ? ? ?} > > - ? ? ? ?base->vlan_tci = flow->vlan_tci; > > - ? ?} > > + ? ?commit_vlan_tci(ctx, flow->vlan_tci); > > > > ? ? if (base->tp_src != flow->tp_src) { > > ? ? ? ? nl_msg_put_be16(odp_actions, ODP_ACTION_ATTR_SET_TP_SRC, > > flow->tp_src); > > @@ -3561,8 +3570,12 @@ compose_actions(struct action_xlate_ctx *ctx, > > uint16_t vlan, > > ? ? dst_set_init(&set); > > ? ? compose_dsts(ctx, vlan, in_bundle, out_bundle, &set); > > ? ? compose_mirror_dsts(ctx, vlan, in_bundle, &set); > > + ? ?if (!set.n) { > > + ? ? ? ?return; > > + ? ?} > > > > ? ? /* Output all the packets we can without having to change the VLAN. */ > > + ? ?commit_odp_actions(ctx); > > ? ? initial_vlan = vlan_tci_to_vid(ctx->flow.vlan_tci); > > ? ? if (initial_vlan == 0) { > > ? ? ? ? initial_vlan = OFP_VLAN_NONE; > > @@ -3582,15 +3595,15 @@ compose_actions(struct action_xlate_ctx *ctx, > > uint16_t vlan, > > ? ? ? ? ? ? continue; > > ? ? ? ? } > > ? ? ? ? if (dst->vlan != cur_vlan) { > > - ? ? ? ? ? ?if (dst->vlan == OFP_VLAN_NONE) { > > - ? ? ? ? ? ? ? ?nl_msg_put_flag(ctx->odp_actions, > > ODP_ACTION_ATTR_STRIP_VLAN); > > - ? ? ? ? ? ?} else { > > - ? ? ? ? ? ? ? ?ovs_be16 tci; > > - ? ? ? ? ? ? ? ?tci = htons(dst->vlan & VLAN_VID_MASK); > > - ? ? ? ? ? ? ? ?tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK); > > - ? ? ? ? ? ? ? ?nl_msg_put_be16(ctx->odp_actions, > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ODP_ACTION_ATTR_SET_DL_TCI, tci); > > + ? ? ? ? ? ?ovs_be16 tci; > > + > > + ? ? ? ? ? ?tci = htons(dst->vlan == OFP_VLAN_NONE ? 0 : dst->vlan); > > + ? ? ? ? ? ?tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK); > > + ? ? ? ? ? ?if (tci) { > > + ? ? ? ? ? ? ? ?tci |= htons(VLAN_CFI); > > ? ? ? ? ? ? } > > + ? ? ? ? ? ?commit_vlan_tci(ctx, tci); > > + > > ? ? ? ? ? ? cur_vlan = dst->vlan; > > ? ? ? ? } > > ? ? ? ? nl_msg_put_u32(ctx->odp_actions, > > -- > > 1.7.4.4 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/dev > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
