[ovs-dev] [PATCH branch-1.2] ofproto-dpif: Fix VLAN and other field handling in OFPP_NORMAL.

[Ben Pfaff]

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 <blp at nicira.com> 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
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >