From: Ben Pfaff <blp@nicira.com>
Date: Sat, 24 Sep 2011 00:11:49 +0000 (-0700)
Subject: ofproto-dpif: Fix VLAN and other field handling in OFPP_NORMAL.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=823518f1f26c2320c35e762c9d1bd29e68f9c5b8;p=openvswitch

ofproto-dpif: Fix VLAN and other field handling in OFPP_NORMAL.

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.

Third, when it switched to VLAN 0, it unconditionally also stripped the
whole 802.1Q header, so that if the packet originally was priority tagged,
the output packet was not priority tagged.  This is reasonable behavior,
but it is a change in behavior from what previous releases of OVS did, so
this commit reverts it.

Based on a patch from and a conversation with Pravin Shelar
<pshelar@nicira.com>.
---

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 47a9d29e..ec5e3be2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2887,6 +2887,26 @@ static void do_xlate_actions(const union ofp_action *in, size_t n_in,
                              struct action_xlate_ctx *ctx);
 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, OVS_ACTION_ATTR_POP_VLAN);
+        } else {
+            if (base->vlan_tci != htons(0)) {
+                nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
+            }
+            nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
+                            vlan_tci & ~htons(VLAN_CFI));
+        }
+        base->vlan_tci = vlan_tci;
+    }
+}
+
 static void
 commit_odp_actions(struct action_xlate_ctx *ctx)
 {
@@ -2914,18 +2934,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, OVS_ACTION_ATTR_POP_VLAN);
-        } else {
-            if (base->vlan_tci != htons(0)) {
-                nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
-            }
-            nl_msg_put_be16(odp_actions, OVS_ACTION_ATTR_PUSH_VLAN,
-                            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, OVS_ACTION_ATTR_SET_TP_SRC, flow->tp_src);
@@ -3741,8 +3750,13 @@ 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) {
+        dst_set_free(&set);
+        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;
@@ -3762,19 +3776,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, OVS_ACTION_ATTR_POP_VLAN);
-            } else {
-                ovs_be16 tci;
+            ovs_be16 tci;
 
-                if (cur_vlan != OFP_VLAN_NONE) {
-                     nl_msg_put_flag(ctx->odp_actions, OVS_ACTION_ATTR_POP_VLAN);
-                }
-                tci = htons(dst->vlan & VLAN_VID_MASK);
-                tci |= ctx->flow.vlan_tci & htons(VLAN_PCP_MASK);
-                nl_msg_put_be16(ctx->odp_actions,
-                                OVS_ACTION_ATTR_PUSH_VLAN, 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,