ofproto: Make NXAST_RESUBMIT take header modifications into account.

Until now, the NXAST_RESUBMIT action has always looked up the original
flow except for the updated in_port.  This commit changes the semantics to
instead look up the flow as modified by any preceding actions that affect
it, e.g. if OFPAT_SET_VLAN_VID precedes NXAST_RESUBMIT, then NXAST_RESUBMIT
now looks up the flow with the modified VLAN, not the original (as well as
the modified in_port).

Also, document how NXAST_RESUBMIT is supposed to work.

Suggested-by: Paul Ingram <paul@nicira.com>

diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 7232d570723c8c54ab23ce95e931e82af67d56bc..535cfc3ae070a6330742bf2029a4c3154375bcdd 100644 (file)
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -57,7 +57,28 @@ OFP_ASSERT(sizeof(struct nicira_header) == 16);
 
 enum nx_action_subtype {
     NXAST_SNAT__OBSOLETE,           /* No longer used. */
-    NXAST_RESUBMIT                  /* Throw against flow table again. */
+
+    /* Searches the flow table again, using a flow that is slightly modified
+     * from the original lookup:
+     *
+     *    - The flow's in_port is changed to that specified in the 'in_port'
+     *      member of struct nx_action_resubmit.
+     *
+     *    - If NXAST_RESUBMIT is preceded by actions that affect the flow
+     *      (e.g. OFPAT_SET_VLAN_VID), then the flow is updated with the new
+     *      values.
+     *
+     * If the modified flow matches in the flow table, then the corresponding
+     * actions are executed, except that NXAST_RESUBMIT actions found in the
+     * secondary set of actions are ignored.  Afterward, actions following
+     * NXAST_RESUBMIT in the original set of actions, if any, are executed; any
+     * changes made to the packet (e.g. changes to VLAN) by secondary actions
+     * persist when those actions are executed, although the original in_port
+     * is restored.
+     *
+     * NXAST_RESUBMIT may be used any number of times within a set of actions.
+     */
+    NXAST_RESUBMIT
 };
 
 /* Action structure for NXAST_RESUBMIT. */

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 9ddf6b155a9670d61a88a5170af68b7b6a56eaf0..0f4ba6844a35d4d2db4134d292c3da81ec97ed16 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2150,6 +2150,9 @@ xlate_nicira_action(struct action_xlate_ctx *ctx,
         xlate_table_action(ctx, ofp_port_to_odp_port(ntohs(nar->in_port)));
         break;
 
+    /* If you add a new action here that modifies flow data, don't forget to
+     * update the flow key in ctx->flow in the same key. */
+
     default:
         VLOG_DBG_RL(&rl, "unknown Nicira action type %"PRIu16, subtype);
         break;
@@ -2183,53 +2186,59 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
 
         case OFPAT_SET_VLAN_VID:
             oa = odp_actions_add(ctx->out, ODPAT_SET_VLAN_VID);
-            oa->vlan_vid.vlan_vid = ia->vlan_vid.vlan_vid;
+            ctx->flow.dl_vlan = oa->vlan_vid.vlan_vid = ia->vlan_vid.vlan_vid;
             break;
 
         case OFPAT_SET_VLAN_PCP:
             oa = odp_actions_add(ctx->out, ODPAT_SET_VLAN_PCP);
-            oa->vlan_pcp.vlan_pcp = ia->vlan_pcp.vlan_pcp;
+            ctx->flow.dl_vlan_pcp = oa->vlan_pcp.vlan_pcp = ia->vlan_pcp.vlan_pcp;
             break;
 
         case OFPAT_STRIP_VLAN:
             odp_actions_add(ctx->out, ODPAT_STRIP_VLAN);
+            ctx->flow.dl_vlan = OFP_VLAN_NONE;
+            ctx->flow.dl_vlan_pcp = 0;
             break;
 
         case OFPAT_SET_DL_SRC:
             oa = odp_actions_add(ctx->out, ODPAT_SET_DL_SRC);
             memcpy(oa->dl_addr.dl_addr,
                    ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
+            memcpy(ctx->flow.dl_src,
+                   ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
             break;
 
         case OFPAT_SET_DL_DST:
             oa = odp_actions_add(ctx->out, ODPAT_SET_DL_DST);
             memcpy(oa->dl_addr.dl_addr,
                    ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
+            memcpy(ctx->flow.dl_dst,
+                   ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
             break;
 
         case OFPAT_SET_NW_SRC:
             oa = odp_actions_add(ctx->out, ODPAT_SET_NW_SRC);
-            oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
+            ctx->flow.nw_src = oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
             break;
 
         case OFPAT_SET_NW_DST:
             oa = odp_actions_add(ctx->out, ODPAT_SET_NW_DST);
-            oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
+            ctx->flow.nw_dst = oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
             break;
 
         case OFPAT_SET_NW_TOS:
             oa = odp_actions_add(ctx->out, ODPAT_SET_NW_TOS);
-            oa->nw_tos.nw_tos = ia->nw_tos.nw_tos;
+            ctx->flow.nw_tos = oa->nw_tos.nw_tos = ia->nw_tos.nw_tos;
             break;
 
         case OFPAT_SET_TP_SRC:
             oa = odp_actions_add(ctx->out, ODPAT_SET_TP_SRC);
-            oa->tp_port.tp_port = ia->tp_port.tp_port;
+            ctx->flow.tp_src = oa->tp_port.tp_port = ia->tp_port.tp_port;
             break;
 
         case OFPAT_SET_TP_DST:
             oa = odp_actions_add(ctx->out, ODPAT_SET_TP_DST);
-            oa->tp_port.tp_port = ia->tp_port.tp_port;
+            ctx->flow.tp_dst = oa->tp_port.tp_port = ia->tp_port.tp_port;
             break;
 
         case OFPAT_VENDOR: