We used to roll back group bucket changes only for 'all' and
'indirect' group types, but the expected semantics of all group types
is that any changes by the group bucket are not visible after the
group has been executed.
Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
---
ofproto/ofproto-dpif-xlate.c | 37 +++++++++++++++++++++++++++++--------
tests/ofproto-dpif.at | 12 ++++++++++++
2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ece9670..8ca35a7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3057,6 +3057,8 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct
ofputil_bucket *bucket)
{
uint64_t action_list_stub[1024 / 8];
struct ofpbuf action_list, action_set;
+ struct flow old_flow = ctx->xin->flow;
+ bool old_was_mpls = ctx->was_mpls;
ofpbuf_use_const(&action_set, bucket->ofpacts, bucket->ofpacts_len);
ofpbuf_use_stub(&action_list, action_list_stub, sizeof action_list_stub);
@@ -3068,6 +3070,33 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct
ofputil_bucket *bucket)
ofpbuf_uninit(&action_set);
ofpbuf_uninit(&action_list);
+
+ /* Roll back flow to previous state.
+ * This is equivalent to cloning the packet for each bucket.
+ *
+ * As a side effect any subsequently applied actions will
+ * also effectively be applied to a clone of the packet taken
+ * just before applying the all or indirect group.
+ *
+ * Note that group buckets are action sets, hence they cannot modify the
+ * main action set. Also any stack actions are ignored when executing an
+ * action set, so group buckets cannot change the stack either.
+ * However, we do allow resubmit actions in group buckets, which could
+ * break the above assumptions. It is upto the controller to not mess up
+ * with the action_set and stack in the tables resubmitted to from
+ * group buckets. */
+ ctx->xin->flow = old_flow;
+
+ /* The group bucket popping MPLS should have no effect after bucket
+ * execution. */
+ ctx->was_mpls = old_was_mpls;
+
+ /* The fact that the group bucket exits (for any reason) does not mean that
+ * the translation after the group action should exit. Specifically, if
+ * the group bucket recirculates (which typically modifies the packet), the
+ * actions after the group action must continue processing with the
+ * original, not the recirculated packet! */
+ ctx->exit = false;
}
static void
@@ -3075,19 +3104,11 @@ xlate_all_group(struct xlate_ctx *ctx, struct
group_dpif *group)
{
struct ofputil_bucket *bucket;
const struct ovs_list *buckets;
- struct flow old_flow = ctx->xin->flow;
group_dpif_get_buckets(group, &buckets);
LIST_FOR_EACH (bucket, list_node, buckets) {
xlate_group_bucket(ctx, bucket);
- /* Roll back flow to previous state.
- * This is equivalent to cloning the packet for each bucket.
- *
- * As a side effect any subsequently applied actions will
- * also effectively be applied to a clone of the packet taken
- * just before applying the all or indirect group. */
- ctx->xin->flow = old_flow;
}
xlate_group_stats(ctx, group, NULL);
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 897df4e..26550c4 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -332,6 +332,18 @@ AT_CHECK([tail -1 stdout], [0],
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - group actions have no effect afterwards])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [10])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0
'group_id=1234,type=select,bucket=set_field:192.168.3.90->ip_src,output:10'])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip
actions=group:1234,output:10'])
+AT_CHECK([ovs-appctl ofproto/trace br0
'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_proto=1,nw_tos=0,nw_ttl=128,icmp_type=8,icmp_code=0'],
[0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+ [Datapath actions:
set(ipv4(src=192.168.3.90,dst=192.168.0.2)),10,set(ipv4(src=192.168.0.1,dst=192.168.0.2)),10
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - all group in action set])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [10], [11])
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
