On Fri, Oct 11, 2013 at 01:23:29PM +0900, Simon Horman wrote:
> Implementation note:
>
> All actions which modify a field are added to the action set
> at the point where "set" actions should be added. In general
> modifying a field many times is the same as only modifying it
> the last time so the implementation simply adds all set actions to
> the action set in the order they are specified. However, this breaks
> down if two actions modify different portions of the same field.
>
> Some examples.
>
> 1. load acting a subfield
> 2. mod_vlan_vid, mod_vlan_pcp
>
> If this is considered to be a problem one possible solution would be to
> either disallow all set actions other than set_field in write_actions.
> Another possible solution is prohibit problematic the actions listed above
> in write actions.
>
> Signed-off-by: Simon Horman <[email protected]>
Thanks.
I did a significant amount of simplification and editing on this.
I'll apply it to master in a minute. Here is the incremental followed
by the full commit.
diff --git a/NEWS b/NEWS
index 94e0da9..891c3fb 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ Post-v2.0.0
IANA-assigned numbers in a future release. Consider updating
your installations to specify port numbers instead of using the
defaults.
+ - OpenFlow:
+ * The OpenFlow 1.1+ "Write-Actions" instruction is now supported.
v2.0.0 - xx xxx xxxx
diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index 90f811f..07b2660 100644
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -54,9 +54,6 @@ OpenFlow 1.1
The list of remaining work items for OpenFlow 1.1 is below. It is
probably incomplete.
- * Implement Write-Actions instruction.
- [required for 1.1+]
-
* The new in_phy_port field in OFPT_PACKET_IN needs some kind of
implementation. It has a sensible interpretation for tunnels
but in general the physical port is not in the datapath for OVS
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index ae1ccf1..06f9f6b 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1005,80 +1005,50 @@ ofpact_copy(struct ofpbuf *out, const struct ofpact *a)
ofpbuf_put(out, a, OFPACT_ALIGN(a->len));
}
-/* Find the last ofpact, whose type is 'filter',
- * from 'in' whose length is 'in_len'. */
-static const struct ofpact *
-ofpacts_find_last(const struct ofpact *in, size_t in_len,
+/* Copies the last ofpact whose type is 'filter' from 'in' to 'out'. */
+static bool
+ofpacts_copy_last(struct ofpbuf *out, const struct ofpbuf *in,
enum ofpact_type filter)
{
+ const struct ofpact *target;
const struct ofpact *a;
- OFPACT_FOR_EACH (a, in, in_len) {
+ target = NULL;
+ OFPACT_FOR_EACH (a, in->data, in->size) {
if (a->type == filter) {
- return a;
+ target = a;
}
}
-
- return NULL;
-}
-
-/* Append the last ofpact, whose type is 'filter', from 'in' to 'out'.
- * In is a list of 'list_node' fields of struct ofpact whose
- * 'data' is serialised be used as a struct ofpact * and whose 'size'
- * field is the length of ofpact data */
-static bool
-ofpacts_list_copy_last(struct ofpbuf *out, const struct list *in,
- enum ofpact_type filter)
-{
- const struct ofpbuf *e;
- const struct ofpact *target = NULL;
-
- LIST_FOR_EACH_REVERSE(e, list_node, in) {
- target = ofpacts_find_last(e->data, e->size, filter);
- if (target) {
- ofpact_copy(out, target);
- return true;
- }
+ if (target) {
+ ofpact_copy(out, target);
}
-
- return false;
+ return target != NULL;
}
/* Append all ofpacts, for which 'filter' returns true, from 'in' to 'out'.
* The order of appended ofpacts is preserved between 'in' and 'out' */
static void
-ofpacts_copy_all(struct ofpbuf *out, const struct ofpact *in,
- size_t in_len, bool (*filter)(const struct ofpact *a))
+ofpacts_copy_all(struct ofpbuf *out, const struct ofpbuf *in,
+ bool (*filter)(const struct ofpact *))
{
const struct ofpact *a;
- OFPACT_FOR_EACH (a, in, in_len) {
+ OFPACT_FOR_EACH (a, in->data, in->size) {
if (filter(a)) {
ofpact_copy(out, a);
}
}
}
-/* Append all ofpacts, for which 'filter' returns true, from 'in' to 'out'.
- * The order of appended ofpacts is preserved between 'in' and 'out'
- * In is a list of 'list_node' fields of struct ofpact whose
- * 'data' is serialised be used as a struct ofpact * and whose 'size'
- * field is the length of ofpact data */
-static void
-ofpacts_list_copy_all(struct ofpbuf *out, const struct list *in,
- bool (*filter)(const struct ofpact *a))
-{
- struct ofpbuf *e;
-
- LIST_FOR_EACH(e, list_node, in) {
- ofpacts_copy_all(out, e->data, e->size, filter);
- }
-}
-/* Convert 'in', a list of ofpacts, into a list of ofpacts which
- * may be applied as an action set.
- * In general this involves appending the last instance of each action
- * that is adimissible in the action set in the order described
- * in the OpenFlow specification.
+/* Reads 'action_set', which contains ofpacts accumulated by
+ * OFPACT_WRITE_ACTIONS instructions, and writes equivalent actions to be
+ * executed directly into 'action_list'. (These names correspond to the
+ * "Action Set" and "Action List" terms used in OpenFlow 1.1+.)
+ *
+ * In general this involves appending the last instance of each action that is
+ * adimissible in the action set in the order described in the OpenFlow
+ * specification.
+ *
* Exceptions:
* + output action is only appended if no group action was present in 'in'.
* + As a simplification all set actions are copied in the order the are
@@ -1088,27 +1058,28 @@ ofpacts_list_copy_all(struct ofpbuf *out, const struct
list *in,
* This has an unwanted side-effect of compsoting multiple
* LOAD_REG actions that touch different regions of the same field. */
void
-ofpacts_list_to_action_set(struct ofpbuf *out, const struct list *in)
+ofpacts_execute_action_set(struct ofpbuf *action_list,
+ const struct ofpbuf *action_set)
{
- /* The order here is important.
- * It corresponds to the order set out for the action set
- * in the OpenFlow specification. */
+ /* The OpenFlow spec "Action Set" section specifies this order. */
+ ofpacts_copy_last(action_list, action_set, OFPACT_STRIP_VLAN);
+ ofpacts_copy_last(action_list, action_set, OFPACT_POP_MPLS);
+ ofpacts_copy_last(action_list, action_set, OFPACT_PUSH_MPLS);
+ ofpacts_copy_last(action_list, action_set, OFPACT_PUSH_VLAN);
+ ofpacts_copy_last(action_list, action_set, OFPACT_DEC_TTL);
+ ofpacts_copy_last(action_list, action_set, OFPACT_DEC_MPLS_TTL);
+ ofpacts_copy_all(action_list, action_set, ofpact_is_set_action);
+ ofpacts_copy_last(action_list, action_set, OFPACT_SET_QUEUE);
- /* XXX: TODO: copy TTL inwards */
- ofpacts_list_copy_last(out, in, OFPACT_STRIP_VLAN);
- ofpacts_list_copy_last(out, in, OFPACT_POP_MPLS);
- /* XXX: TODO: Pop BPP */
- ofpacts_list_copy_last(out, in, OFPACT_PUSH_MPLS);
- /* XXX: TODO: Push BPP */
- ofpacts_list_copy_last(out, in, OFPACT_PUSH_VLAN);
- /* XXX: TODO: copy TTL inwards */
- ofpacts_list_copy_last(out, in, OFPACT_DEC_TTL);
- ofpacts_list_copy_last(out, in, OFPACT_DEC_MPLS_TTL);
- ofpacts_list_copy_all(out, in, ofpact_is_set_action);
- ofpacts_list_copy_last(out, in, OFPACT_SET_QUEUE);
- if (!ofpacts_list_copy_last(out, in, OFPACT_GROUP)) {
- /* Output action is only used if there is no group action */
- ofpacts_list_copy_last(out, in, OFPACT_OUTPUT);
+ /* If both OFPACT_GROUP and OFPACT_OUTPUT are present, OpenFlow says that
+ * we should execute only OFPACT_GROUP.
+ *
+ * If neither OFPACT_GROUP nor OFPACT_OUTPUT is present, then we can drop
+ * all the actions because there's no point in modifying a packet that will
+ * not be sent anywhere. */
+ if (!ofpacts_copy_last(action_list, action_set, OFPACT_GROUP) &&
+ !ofpacts_copy_last(action_list, action_set, OFPACT_OUTPUT)) {
+ ofpbuf_clear(action_list);
}
}
@@ -1435,7 +1406,7 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf
*openflow,
size_t start = ofpacts->size;
on = ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS,
- offsetof(struct ofpact_nest, actions));
+ offsetof(struct ofpact_nest, actions));
get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACTIONS],
&actions, &n_actions);
error = ofpacts_from_openflow11_for_action_set(actions, n_actions,
@@ -1474,10 +1445,6 @@ exit:
return error;
}
-enum ofperr
-ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
- struct flow *flow, ofp_port_t max_ports, uint8_t table_id);
-
/* May modify flow->dl_type, caller must restore it. */
static enum ofperr
ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
@@ -1634,7 +1601,9 @@ ofpacts_verify(const struct ofpact ofpacts[], size_t
ofpacts_len)
enum ovs_instruction_type next;
next = ovs_instruction_type_from_ofpact_type(a->type);
- if (inst != OVSINST_OFPIT11_APPLY_ACTIONS && next <= inst) {
+ if (inst == OVSINST_OFPIT11_APPLY_ACTIONS
+ ? next < inst
+ : next <= inst) {
const char *name = ovs_instruction_name_from_type(inst);
const char *next_name = ovs_instruction_name_from_type(next);
@@ -2491,11 +2460,7 @@ print_fin_timeout(const struct ofpact_fin_timeout
*fin_timeout,
}
static void
-ofpacts_format__(const struct ofpact *ofpacts, size_t ofpacts_len,
- struct ds *string, int loop);
-
-static void
-ofpact_format(const struct ofpact *a, struct ds *s, int loop)
+ofpact_format(const struct ofpact *a, struct ds *s)
{
const struct ofpact_enqueue *enqueue;
const struct ofpact_resubmit *resubmit;
@@ -2721,7 +2686,7 @@ ofpact_format(const struct ofpact *a, struct ds *s, int
loop)
ds_put_format(s, "%s(",
ovs_instruction_name_from_type(
OVSINST_OFPIT11_WRITE_ACTIONS));
- ofpacts_format__(on->actions, ofpact_nest_get_action_len(on), s, loop);
+ ofpacts_format(on->actions, ofpact_nest_get_action_len(on), s);
ds_put_char(s, ')');
break;
}
@@ -2763,19 +2728,12 @@ ofpact_format(const struct ofpact *a, struct ds *s, int
loop)
}
}
-#define OFPACTS_FORMAT_RECURSION_LIMIT 2 /* Sufficient for one instance of
- * Write Actions */
-
/* Appends a string representing the 'ofpacts_len' bytes of ofpacts in
* 'ofpacts' to 'string'. */
-static void
-ofpacts_format__(const struct ofpact *ofpacts, size_t ofpacts_len,
- struct ds *string, int loop)
+void
+ofpacts_format(const struct ofpact *ofpacts, size_t ofpacts_len,
+ struct ds *string)
{
- if (loop++ > OFPACTS_FORMAT_RECURSION_LIMIT) {
- NOT_REACHED();
- }
-
if (!ofpacts_len) {
ds_put_cstr(string, "drop");
} else {
@@ -2787,20 +2745,10 @@ ofpacts_format__(const struct ofpact *ofpacts, size_t
ofpacts_len,
}
/* XXX write-actions */
- ofpact_format(a, string, loop);
+ ofpact_format(a, string);
}
}
}
-
-/* Appends a string representing the 'ofpacts_len' bytes of ofpacts in
- * 'ofpacts' to 'string'. */
-void
-ofpacts_format(const struct ofpact *ofpacts, size_t ofpacts_len,
- struct ds *string)
-{
- ds_put_cstr(string, "actions=");
- ofpacts_format__(ofpacts, ofpacts_len, string, 0);
-}
/* Internal use by helpers. */
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 129d6e8..5996651 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -17,6 +17,7 @@
#ifndef OFP_ACTIONS_H
#define OFP_ACTIONS_H 1
+#include <stddef.h>
#include <stdint.h>
#include "meta-flow.h"
#include "ofp-errors.h"
@@ -392,6 +393,16 @@ struct ofpact_nest {
uint8_t pad[OFPACT_ALIGN(sizeof(struct ofpact)) - sizeof(struct ofpact)];
struct ofpact actions[];
};
+BUILD_ASSERT_DECL(offsetof(struct ofpact_nest, actions) == OFPACT_ALIGNTO);
+
+static inline size_t
+ofpact_nest_get_action_len(const struct ofpact_nest *on)
+{
+ return on->ofpact.len - offsetof(struct ofpact_nest, actions);
+}
+
+void ofpacts_execute_action_set(struct ofpbuf *action_list,
+ const struct ofpbuf *action_set);
/* OFPACT_RESUBMIT.
*
@@ -508,16 +519,6 @@ struct ofpact_group {
uint32_t group_id;
};
-/* Helper */
-static inline size_t
-ofpact_nest_get_action_len(const struct ofpact_nest *on)
-{
- return on->ofpact.len - offsetof(typeof(*on), actions);
-}
-
-/* Convert Action List to Action Set */
-void ofpacts_list_to_action_set(struct ofpbuf *out, const struct list *in);
-
/* Converting OpenFlow to ofpacts. */
enum ofperr ofpacts_pull_openflow10(struct ofpbuf *openflow,
unsigned int actions_len,
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index e9b97e9..f55eb3f 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -951,14 +951,17 @@ parse_named_instruction(enum ovs_instruction_type type,
case OVSINST_OFPIT11_WRITE_ACTIONS: {
struct ofpact_nest *on;
- size_t orig_size = ofpacts->size;
+ size_t ofs = ofpacts->size;
on = ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS,
- offsetof(struct ofpact_nest, actions));
+ offsetof(struct ofpact_nest, actions));
error_s = str_to_ofpacts__(arg, ofpacts, usable_protocols);
- on->ofpact.len = ofpacts->size - orig_size;
+
+ on = ofpbuf_at_assert(ofpacts, ofs, sizeof *on);
+ on->ofpact.len = ofpacts->size - ofs;
+
if (error_s) {
- ofpacts->size = orig_size;
+ ofpacts->size = ofs;
}
break;
}
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 33b962f..9313a7f 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -180,9 +180,14 @@ struct xlate_ctx {
uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
bool exit; /* No further actions should be processed. */
- struct list action_set; /* Action set.
- * An ordered list of struct ofpbuf
- * containing ofpacts to add to the action. */
+ /* OpenFlow 1.1+ action set.
+ *
+ * 'action_set' accumulates "struct ofpact"s added by OFPACT_WRITE_ACTIONS.
+ * When translation is otherwise complete, ofpacts_execute_action_set()
+ * converts it to a set of "struct ofpact"s that can be translated into
+ * datapath actions. */
+ struct ofpbuf action_set; /* Action set. */
+ uint64_t action_set_stub[1024 / 8];
};
/* A controller may use OFPP_NONE as the ingress port to indicate that
@@ -2251,50 +2256,23 @@ may_receive(const struct xport *xport, struct xlate_ctx
*ctx)
}
static void
-action_set_clear(struct xlate_ctx *ctx)
-{
- ofpbuf_list_delete(&ctx->action_set);
-}
-
-static void
-action_set_add(struct xlate_ctx *ctx, struct ofpact *ofpacts,
- size_t ofpacts_len)
-{
- struct ofpbuf *new = xmalloc(sizeof *new);
-
- ofpbuf_use_const(new, ofpacts, ofpacts_len);
- list_init(&new->list_node);
-
- list_push_back(&ctx->action_set, &new->list_node);
-}
-
-static void
xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact *a)
{
struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
- action_set_add(ctx, on->actions, ofpact_nest_get_action_len(on));
+ ofpbuf_put(&ctx->action_set, on->actions, ofpact_nest_get_action_len(on));
+ ofpact_pad(&ctx->action_set);
}
static void
xlate_action_set(struct xlate_ctx *ctx)
{
- struct ofpbuf ofpact_buf;
+ uint64_t action_list_stub[1024 / 64];
+ struct ofpbuf action_list;
- if (ctx->exit) {
- action_set_clear(ctx);
- }
-
- if (list_is_empty(&ctx->action_set)) {
- return;
- }
-
- ofpbuf_init(&ofpact_buf, 64);
- ofpacts_list_to_action_set(&ofpact_buf, &ctx->action_set);
-
- action_set_clear(ctx);
- do_xlate_actions(ofpact_buf.data, ofpact_buf.size, ctx);
-
- ofpbuf_uninit(&ofpact_buf);
+ ofpbuf_use_stub(&action_list, action_list_stub, sizeof action_list_stub);
+ ofpacts_execute_action_set(&action_list, &ctx->action_set);
+ do_xlate_actions(action_list.data, action_list.size, ctx);
+ ofpbuf_uninit(&action_list);
}
static void
@@ -2508,7 +2486,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
case OFPACT_CLEAR_ACTIONS:
- action_set_clear(ctx);
+ ofpbuf_clear(&ctx->action_set);
break;
case OFPACT_WRITE_ACTIONS:
@@ -2532,9 +2510,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
ovs_assert(ctx->table_id < ogt->table_id);
xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
ogt->table_id, true);
- /* Just in case xlate_action_set() wasn't called indirectly
- * by xlate_table_action() */
- action_set_clear(ctx);
break;
}
@@ -2543,8 +2518,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
}
}
-
- xlate_action_set(ctx);
}
void
@@ -2793,7 +2766,6 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
ctx.table_id = 0;
ctx.exit = false;
ctx.mpls_depth_delta = 0;
- list_init(&ctx.action_set);
if (!xin->ofpacts && !ctx.rule) {
rule_dpif_lookup(ctx.xbridge->ofproto, flow, wc, &rule);
@@ -2816,6 +2788,8 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
}
ofpbuf_use_stub(&ctx.stack, ctx.init_stack, sizeof ctx.init_stack);
+ ofpbuf_use_stub(&ctx.action_set,
+ ctx.action_set_stub, sizeof ctx.action_set_stub);
if (mbridge_has_mirrors(ctx.xbridge->mbridge)) {
/* Do this conditionally because the copy is expensive enough that it
@@ -2874,6 +2848,10 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
}
}
+ if (ctx.action_set.size) {
+ xlate_action_set(&ctx);
+ }
+
if (ctx.xbridge->has_in_band
&& in_band_must_output_to_local_port(flow)
&& !actions_output_to_local_port(&ctx)) {
@@ -2897,6 +2875,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
}
ofpbuf_uninit(&ctx.stack);
+ ofpbuf_uninit(&ctx.action_set);
/* Clear the metadata and register wildcard masks, because we won't
* use non-header fields as part of the cache. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index cee6f2d..3630cda 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -37,9 +37,11 @@ AT_CLEANUP
AT_SETUP([ofproto-dpif - write actions])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [10], [11], [12], [13])
-echo "table=0 in_port=1,ip
actions=output(10),write_actions(set_field:192.168.3.90->ip_src,output(12)),goto_table(1)"
> flows.txt
-echo "table=1 ip actions=write_actions(output(13)),goto_table(2)" >> flows.txt
-echo "table=2 ip actions=set_field:192.168.3.91->ip_src,output(11)" >>
flows.txt
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1,ip
actions=output(10),write_actions(set_field:192.168.3.90->ip_src,output(12)),goto_table(1)
+table=1 ip actions=write_actions(output(13)),goto_table(2)
+table=2 ip actions=set_field:192.168.3.91->ip_src,output(11)
+])
AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt])
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],
@@ -51,8 +53,10 @@ AT_CLEANUP
AT_SETUP([ofproto-dpif - clear actions])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [10], [11], [12])
-echo "table=0 in_port=1,ip
actions=output(10),write_actions(set_field:192.168.3.90->ip_src,output(12)),goto_table(1)"
> flows.txt
-echo "table=1 ip
actions=set_field:192.168.3.91->ip_src,output(11),clear_actions" >> flows.txt
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1,ip
actions=output(10),write_actions(set_field:192.168.3.90->ip_src,output(12)),goto_table(1)
+table=1 ip actions=set_field:192.168.3.91->ip_src,output(11),clear_actions
+])
AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt])
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],
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 400f10a..b2bc8d7 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1347,55 +1347,85 @@ to \fBactions=\fR field.
Clears all the actions in the action set immediately.
.
.IP \fBwrite_actions(\fR[\fIaction\fR][\fB,\fIaction\fR...]\fB)
-Append the specific action(s) to the action set. The syntax of actions are same
-to \fBactions=\fR field. The action set may be cleared using
-\fBclear_actions\fR. The action set carried between flow tables.
-The actions in the action set are executed when the actions of a flow do
-not include a \fBgoto_table\fR action and there
-is no \fBexit\fR action persent in the \fBresubmit\fR call stack.
+Add the specific actions to the action set. The syntax of
+\fIactions\fR is the same as in the \fBactions=\fR field. The action
+set is carried between flow tables and then executed at the end of the
+pipeline.
.
-The actions in the action set are applied in the following order:
-.
-.IP
-\fBPOP actions: \fBstrip_vlan\fR and \fBpop_mpls\fR.
-The pop actions may be applied in any order relative to each other.
.IP
+The actions in the action set are applied in the following order, as
+required by the OpenFlow specification, regardless of the order in
+which they were added to the action set. Except as specified
+otherwise below, the action set only holds at most a single action of
+each type. When more than one action of a single type is written to
+the action set, the one written later replaces the earlier action:
+.
+.RS
+.IP 1.
+\fBstrip_vlan\fR
+.IQ
+\fBpop_mpls\fR
+.
+.IP 2.
\fBpush_mpls\fR
-.IP
+.
+.IP 3.
\fBpush_vlan\fR
-.IP
-\fBDecrement TTL actions\fR: \fBdec_ttl\fR and \fBdec_mpls_ttl\fR.
-The decrement actions may be applied in any order relative to each other.
-.IP
-\fBSet actions\fR.
-Where multiple set actions modify the same field of same part of a field
-the last modification takes effect. Internally set actions are all
-applied. This is in keeping with the desired set action behaviour. But
-has an unwanted side effect of compositing multiple actions if they
-modify different regions of the same field.
-The following are considered set actions:
-.IP
- \fBload\fR
- \fBmod_dl_dst\fR
- \fBmod_dl_src\fR
- \fBmod_nw_dst\fR
- \fBmod_nw_src\fR
- \fBmod_nw_tos\fR
- \fBmod_tp_dst\fR
- \fBmod_tp_src\fR
- \fBmod_vlan_pcp\fR
- \fBmod_vlan_vid\fR
- \fBset_field\fR
- \fBset_tunnel64\fR
- \fBset_tunnel\fR
-.RE
-.IP
+.
+.IP 4.
+\fBdec_ttl\fR
+.IQ
+\fBdec_mpls_ttl\fR
+.
+.IP 5.
+\fBload\fR
+.IQ
+\fBmod_dl_dst\fR
+.IQ
+\fBmod_dl_src\fR
+.IQ
+\fBmod_nw_dst\fR
+.IQ
+\fBmod_nw_src\fR
+.IQ
+\fBmod_nw_tos\fR
+.IQ
+\fBmod_tp_dst\fR
+.IQ
+\fBmod_tp_src\fR
+.IQ
+\fBmod_vlan_pcp\fR
+.IQ
+\fBmod_vlan_vid\fR
+.IQ
+\fBset_field\fR
+.IQ
+\fBset_tunnel\fR
+.IQ
+\fBset_tunnel64\fR
+.IQ
+The action set can contain any number of these actions, with
+cumulative effect. That is, when multiple actions modify the same
+part of a field, the later modification takes effect, and when they
+modify different parts of a field (or different fields), then both
+modifications are applied.
+.
+.IP 6.
\fBset_queue\fR
-.IP
+.
+.IP 7.
\fBgroup\fR
-.IP
+.IQ
\fBoutput\fR
-Not added if a group action is present in the action set.
+.IQ
+If both actions are present, then \fBgroup\fR is executed and
+\fBoutput\fR is ignored, regardless of the order in which they were
+added to the action set. (If neither action is present, the action
+set has no real effect, because the modified packet is not sent
+anywhere and thus the modifications are not visible.)
+.RE
+.IP
+Only the actions listed above may be written to the action set.
.
.IP \fBwrite_metadata\fB:\fIvalue\fR[/\fImask\fR]
Updates the metadata field for the flow. If \fImask\fR is omitted, the
@@ -1460,10 +1490,12 @@ configuring sample collector sets.
This action was added in Open vSwitch 1.10.90.
.
.IP "\fBexit\fR"
-This action causes Open vSwitch to immediately halt execution of further
-actions. Those actions which have already been executed are unaffected. Any
-further actions, including those which may be in other tables, or different
-levels of the \fBresubmit\fR call stack, are ignored.
+This action causes Open vSwitch to immediately halt execution of
+further actions. Those actions which have already been executed are
+unaffected. Any further actions, including those which may be in
+other tables, or different levels of the \fBresubmit\fR call stack,
+are ignored. Actions in the action set is still executed (specify
+\fBclear_actions\fR before \fBexit\fR to discard them).
.RE
.
.PP
--
1.7.10.4
--8<--------------------------cut here-------------------------->8--
From: Simon Horman <[email protected]>
Date: Fri, 11 Oct 2013 13:23:29 +0900
Subject: [PATCH] Add support for write-actions
Implementation note:
All actions which modify a field are added to the action set
at the point where "set" actions should be added. In general
modifying a field many times is the same as only modifying it
the last time so the implementation simply adds all set actions to
the action set in the order they are specified. However, this breaks
down if two actions modify different portions of the same field.
Some examples.
1. load acting a subfield
2. mod_vlan_vid, mod_vlan_pcp
If this is considered to be a problem one possible solution would be to
either disallow all set actions other than set_field in write_actions.
Another possible solution is prohibit problematic the actions listed above
in write actions.
Signed-off-by: Simon Horman <[email protected]>
[[email protected] simplified and edited the code]
Signed-off-by: Ben Pfaff <[email protected]>
---
NEWS | 2 +
OPENFLOW-1.1+ | 3 -
lib/ofp-actions.c | 293 ++++++++++++++++++++++++++++++++++++++++--
lib/ofp-actions.h | 23 +++-
lib/ofp-parse.c | 46 ++++++-
ofproto/ofproto-dpif-xlate.c | 46 ++++++-
tests/ofp-actions.at | 26 +++-
tests/ofproto-dpif.at | 31 +++++
utilities/ovs-ofctl.8.in | 91 ++++++++++++-
9 files changed, 528 insertions(+), 33 deletions(-)
diff --git a/NEWS b/NEWS
index 94e0da9..891c3fb 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ Post-v2.0.0
IANA-assigned numbers in a future release. Consider updating
your installations to specify port numbers instead of using the
defaults.
+ - OpenFlow:
+ * The OpenFlow 1.1+ "Write-Actions" instruction is now supported.
v2.0.0 - xx xxx xxxx
diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index 90f811f..07b2660 100644
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -54,9 +54,6 @@ OpenFlow 1.1
The list of remaining work items for OpenFlow 1.1 is below. It is
probably incomplete.
- * Implement Write-Actions instruction.
- [required for 1.1+]
-
* The new in_phy_port field in OFPT_PACKET_IN needs some kind of
implementation. It has a sensible interpretation for tunnels
but in general the physical port is not in the datapath for OVS
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 1d0321a..06f9f6b 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -880,6 +880,233 @@ ofpacts_from_openflow11(const union ofp_action *in,
size_t n_in,
{
return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow11);
}
+
+/* True if an action sets the value of a field
+ * in a way that is compatibile with the action set.
+ * False otherwise. */
+static bool
+ofpact_is_set_action(const struct ofpact *a)
+{
+ switch (a->type) {
+ case OFPACT_REG_LOAD:
+ case OFPACT_SET_ETH_DST:
+ case OFPACT_SET_ETH_SRC:
+ case OFPACT_SET_IPV4_DSCP:
+ case OFPACT_SET_IPV4_DST:
+ case OFPACT_SET_IPV4_SRC:
+ case OFPACT_SET_L4_DST_PORT:
+ case OFPACT_SET_L4_SRC_PORT:
+ case OFPACT_SET_MPLS_TTL:
+ case OFPACT_SET_QUEUE:
+ case OFPACT_SET_TUNNEL:
+ case OFPACT_SET_VLAN_PCP:
+ case OFPACT_SET_VLAN_VID:
+ return true;
+ case OFPACT_BUNDLE:
+ case OFPACT_CLEAR_ACTIONS:
+ case OFPACT_CONTROLLER:
+ case OFPACT_DEC_MPLS_TTL:
+ case OFPACT_DEC_TTL:
+ case OFPACT_ENQUEUE:
+ case OFPACT_EXIT:
+ case OFPACT_FIN_TIMEOUT:
+ case OFPACT_GOTO_TABLE:
+ case OFPACT_GROUP:
+ case OFPACT_LEARN:
+ case OFPACT_METER:
+ case OFPACT_MULTIPATH:
+ case OFPACT_NOTE:
+ case OFPACT_OUTPUT:
+ case OFPACT_OUTPUT_REG:
+ case OFPACT_POP_MPLS:
+ case OFPACT_POP_QUEUE:
+ case OFPACT_PUSH_MPLS:
+ case OFPACT_PUSH_VLAN:
+ case OFPACT_REG_MOVE:
+ case OFPACT_RESUBMIT:
+ case OFPACT_SAMPLE:
+ case OFPACT_STACK_POP:
+ case OFPACT_STACK_PUSH:
+ case OFPACT_STRIP_VLAN:
+ case OFPACT_WRITE_ACTIONS:
+ case OFPACT_WRITE_METADATA:
+ return false;
+ default:
+ NOT_REACHED();
+ }
+}
+
+/* True if an action is allowed in the action set.
+ * False otherwise. */
+static bool
+ofpact_is_allowed_in_actions_set(const struct ofpact *a)
+{
+ switch (a->type) {
+ case OFPACT_DEC_MPLS_TTL:
+ case OFPACT_DEC_TTL:
+ case OFPACT_GROUP:
+ case OFPACT_OUTPUT:
+ case OFPACT_POP_MPLS:
+ case OFPACT_PUSH_MPLS:
+ case OFPACT_PUSH_VLAN:
+ case OFPACT_REG_LOAD:
+ case OFPACT_SET_ETH_DST:
+ case OFPACT_SET_ETH_SRC:
+ case OFPACT_SET_IPV4_DSCP:
+ case OFPACT_SET_IPV4_DST:
+ case OFPACT_SET_IPV4_SRC:
+ case OFPACT_SET_L4_DST_PORT:
+ case OFPACT_SET_L4_SRC_PORT:
+ case OFPACT_SET_MPLS_TTL:
+ case OFPACT_SET_QUEUE:
+ case OFPACT_SET_TUNNEL:
+ case OFPACT_SET_VLAN_PCP:
+ case OFPACT_SET_VLAN_VID:
+ case OFPACT_STRIP_VLAN:
+ return true;
+
+ /* In general these actions are excluded because they are not part of
+ * the OpenFlow specification nor map to actions that are defined in
+ * the specification. Thus the order in which they should be applied
+ * in the action set is undefined. */
+ case OFPACT_BUNDLE:
+ case OFPACT_CONTROLLER:
+ case OFPACT_ENQUEUE:
+ case OFPACT_EXIT:
+ case OFPACT_FIN_TIMEOUT:
+ case OFPACT_LEARN:
+ case OFPACT_MULTIPATH:
+ case OFPACT_NOTE:
+ case OFPACT_OUTPUT_REG:
+ case OFPACT_POP_QUEUE:
+ case OFPACT_REG_MOVE:
+ case OFPACT_RESUBMIT:
+ case OFPACT_SAMPLE:
+ case OFPACT_STACK_POP:
+ case OFPACT_STACK_PUSH:
+
+ /* The action set may only include actions and thus
+ * may not include any instructions */
+ case OFPACT_CLEAR_ACTIONS:
+ case OFPACT_GOTO_TABLE:
+ case OFPACT_METER:
+ case OFPACT_WRITE_ACTIONS:
+ case OFPACT_WRITE_METADATA:
+ return false;
+ default:
+ NOT_REACHED();
+ }
+}
+
+/* Append ofpact 'a' onto the tail of 'out' */
+static void
+ofpact_copy(struct ofpbuf *out, const struct ofpact *a)
+{
+ ofpbuf_put(out, a, OFPACT_ALIGN(a->len));
+}
+
+/* Copies the last ofpact whose type is 'filter' from 'in' to 'out'. */
+static bool
+ofpacts_copy_last(struct ofpbuf *out, const struct ofpbuf *in,
+ enum ofpact_type filter)
+{
+ const struct ofpact *target;
+ const struct ofpact *a;
+
+ target = NULL;
+ OFPACT_FOR_EACH (a, in->data, in->size) {
+ if (a->type == filter) {
+ target = a;
+ }
+ }
+ if (target) {
+ ofpact_copy(out, target);
+ }
+ return target != NULL;
+}
+
+/* Append all ofpacts, for which 'filter' returns true, from 'in' to 'out'.
+ * The order of appended ofpacts is preserved between 'in' and 'out' */
+static void
+ofpacts_copy_all(struct ofpbuf *out, const struct ofpbuf *in,
+ bool (*filter)(const struct ofpact *))
+{
+ const struct ofpact *a;
+
+ OFPACT_FOR_EACH (a, in->data, in->size) {
+ if (filter(a)) {
+ ofpact_copy(out, a);
+ }
+ }
+}
+
+/* Reads 'action_set', which contains ofpacts accumulated by
+ * OFPACT_WRITE_ACTIONS instructions, and writes equivalent actions to be
+ * executed directly into 'action_list'. (These names correspond to the
+ * "Action Set" and "Action List" terms used in OpenFlow 1.1+.)
+ *
+ * In general this involves appending the last instance of each action that is
+ * adimissible in the action set in the order described in the OpenFlow
+ * specification.
+ *
+ * Exceptions:
+ * + output action is only appended if no group action was present in 'in'.
+ * + As a simplification all set actions are copied in the order the are
+ * provided in 'in' as many set actions applied to a field has the same
+ * affect as only applying the last action that sets a field and
+ * duplicates are removed by do_xlate_actions().
+ * This has an unwanted side-effect of compsoting multiple
+ * LOAD_REG actions that touch different regions of the same field. */
+void
+ofpacts_execute_action_set(struct ofpbuf *action_list,
+ const struct ofpbuf *action_set)
+{
+ /* The OpenFlow spec "Action Set" section specifies this order. */
+ ofpacts_copy_last(action_list, action_set, OFPACT_STRIP_VLAN);
+ ofpacts_copy_last(action_list, action_set, OFPACT_POP_MPLS);
+ ofpacts_copy_last(action_list, action_set, OFPACT_PUSH_MPLS);
+ ofpacts_copy_last(action_list, action_set, OFPACT_PUSH_VLAN);
+ ofpacts_copy_last(action_list, action_set, OFPACT_DEC_TTL);
+ ofpacts_copy_last(action_list, action_set, OFPACT_DEC_MPLS_TTL);
+ ofpacts_copy_all(action_list, action_set, ofpact_is_set_action);
+ ofpacts_copy_last(action_list, action_set, OFPACT_SET_QUEUE);
+
+ /* If both OFPACT_GROUP and OFPACT_OUTPUT are present, OpenFlow says that
+ * we should execute only OFPACT_GROUP.
+ *
+ * If neither OFPACT_GROUP nor OFPACT_OUTPUT is present, then we can drop
+ * all the actions because there's no point in modifying a packet that will
+ * not be sent anywhere. */
+ if (!ofpacts_copy_last(action_list, action_set, OFPACT_GROUP) &&
+ !ofpacts_copy_last(action_list, action_set, OFPACT_OUTPUT)) {
+ ofpbuf_clear(action_list);
+ }
+}
+
+
+static enum ofperr
+ofpacts_from_openflow11_for_action_set(const union ofp_action *in,
+ size_t n_in, struct ofpbuf *out)
+{
+ enum ofperr error;
+ struct ofpact *a;
+ size_t start = out->size;
+
+ error = ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow11);
+ if (error) {
+ return error;
+ }
+
+ OFPACT_FOR_EACH (a, ofpact_end(out->data, start), out->size - start) {
+ if (!ofpact_is_allowed_in_actions_set(a)) {
+ VLOG_WARN_RL(&rl, "disallowed action in action set");
+ return OFPERR_OFPBAC_BAD_TYPE;
+ }
+ }
+
+ return 0;
+}
+
/* OpenFlow 1.1 instructions. */
@@ -946,6 +1173,8 @@ ovs_instruction_type_from_ofpact_type(enum ofpact_type
type)
return OVSINST_OFPIT13_METER;
case OFPACT_CLEAR_ACTIONS:
return OVSINST_OFPIT11_CLEAR_ACTIONS;
+ case OFPACT_WRITE_ACTIONS:
+ return OVSINST_OFPIT11_WRITE_ACTIONS;
case OFPACT_WRITE_METADATA:
return OVSINST_OFPIT11_WRITE_METADATA;
case OFPACT_GOTO_TABLE:
@@ -1170,7 +1399,23 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf
*openflow,
insts[OVSINST_OFPIT11_CLEAR_ACTIONS]);
ofpact_put_CLEAR_ACTIONS(ofpacts);
}
- /* XXX Write-Actions */
+ if (insts[OVSINST_OFPIT11_WRITE_ACTIONS]) {
+ struct ofpact_nest *on;
+ const union ofp_action *actions;
+ size_t n_actions;
+ size_t start = ofpacts->size;
+
+ on = ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS,
+ offsetof(struct ofpact_nest, actions));
+ get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACTIONS],
+ &actions, &n_actions);
+ error = ofpacts_from_openflow11_for_action_set(actions, n_actions,
+ ofpacts);
+ if (error) {
+ goto exit;
+ }
+ on->ofpact.len = ofpacts->size - start;
+ }
if (insts[OVSINST_OFPIT11_WRITE_METADATA]) {
const struct ofp11_instruction_write_metadata *oiwm;
struct ofpact_metadata *om;
@@ -1192,11 +1437,6 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf
*openflow,
ogt->table_id = oigt->table_id;
}
- if (insts[OVSINST_OFPIT11_WRITE_ACTIONS]) {
- error = OFPERR_OFPBIC_UNSUP_INST;
- goto exit;
- }
-
error = ofpacts_verify(ofpacts->data, ofpacts->size);
exit:
if (error) {
@@ -1292,6 +1532,14 @@ ofpact_check__(const struct ofpact *a, struct flow
*flow, ofp_port_t max_ports,
return 0;
case OFPACT_CLEAR_ACTIONS:
+ return 0;
+
+ case OFPACT_WRITE_ACTIONS: {
+ struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
+ return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
+ flow, max_ports, table_id);
+ }
+
case OFPACT_WRITE_METADATA:
return 0;
@@ -1353,7 +1601,9 @@ ofpacts_verify(const struct ofpact ofpacts[], size_t
ofpacts_len)
enum ovs_instruction_type next;
next = ovs_instruction_type_from_ofpact_type(a->type);
- if (inst != OVSINST_OFPIT11_APPLY_ACTIONS && next <= inst) {
+ if (inst == OVSINST_OFPIT11_APPLY_ACTIONS
+ ? next < inst
+ : next <= inst) {
const char *name = ovs_instruction_name_from_type(inst);
const char *next_name = ovs_instruction_name_from_type(next);
@@ -1622,6 +1872,7 @@ ofpact_to_nxast(const struct ofpact *a, struct ofpbuf
*out)
case OFPACT_SET_IPV4_DSCP:
case OFPACT_SET_L4_SRC_PORT:
case OFPACT_SET_L4_DST_PORT:
+ case OFPACT_WRITE_ACTIONS:
case OFPACT_CLEAR_ACTIONS:
case OFPACT_GOTO_TABLE:
case OFPACT_METER:
@@ -1716,6 +1967,7 @@ ofpact_to_openflow10(const struct ofpact *a, struct
ofpbuf *out)
case OFPACT_PUSH_VLAN:
case OFPACT_CLEAR_ACTIONS:
+ case OFPACT_WRITE_ACTIONS:
case OFPACT_GOTO_TABLE:
case OFPACT_METER:
/* XXX */
@@ -1892,6 +2144,7 @@ ofpact_to_openflow11(const struct ofpact *a, struct
ofpbuf *out)
break;
case OFPACT_CLEAR_ACTIONS:
+ case OFPACT_WRITE_ACTIONS:
case OFPACT_GOTO_TABLE:
case OFPACT_METER:
NOT_REACHED();
@@ -2016,8 +2269,19 @@ ofpacts_put_openflow11_instructions(const struct ofpact
ofpacts[],
break;
}
- case OVSINST_OFPIT11_WRITE_ACTIONS:
- NOT_REACHED();
+ case OVSINST_OFPIT11_WRITE_ACTIONS: {
+ const size_t ofs = openflow->size;
+ const struct ofpact_nest *on;
+
+ on = ofpact_get_WRITE_ACTIONS(a);
+ instruction_put_OFPIT11_WRITE_ACTIONS(openflow);
+ ofpacts_put_openflow11_actions(on->actions,
+ ofpact_nest_get_action_len(on),
+ openflow);
+ ofpacts_update_instruction_actions(openflow, ofs);
+
+ break;
+ }
}
}
}
@@ -2068,6 +2332,7 @@ ofpact_outputs_to_port(const struct ofpact *ofpact,
ofp_port_t port)
case OFPACT_POP_MPLS:
case OFPACT_SAMPLE:
case OFPACT_CLEAR_ACTIONS:
+ case OFPACT_WRITE_ACTIONS:
case OFPACT_GOTO_TABLE:
case OFPACT_METER:
case OFPACT_GROUP:
@@ -2416,6 +2681,16 @@ ofpact_format(const struct ofpact *a, struct ds *s)
sample->obs_domain_id, sample->obs_point_id);
break;
+ case OFPACT_WRITE_ACTIONS: {
+ struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
+ ds_put_format(s, "%s(",
+ ovs_instruction_name_from_type(
+ OVSINST_OFPIT11_WRITE_ACTIONS));
+ ofpacts_format(on->actions, ofpact_nest_get_action_len(on), s);
+ ds_put_char(s, ')');
+ break;
+ }
+
case OFPACT_CLEAR_ACTIONS:
ds_put_format(s, "%s",
ovs_instruction_name_from_type(
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 0876ed7..5996651 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -17,6 +17,7 @@
#ifndef OFP_ACTIONS_H
#define OFP_ACTIONS_H 1
+#include <stddef.h>
#include <stdint.h>
#include "meta-flow.h"
#include "ofp-errors.h"
@@ -99,8 +100,8 @@
\
/* Instructions */ \
DEFINE_OFPACT(METER, ofpact_meter, ofpact) \
- /* XXX Write-Actions */ \
DEFINE_OFPACT(CLEAR_ACTIONS, ofpact_null, ofpact) \
+ DEFINE_OFPACT(WRITE_ACTIONS, ofpact_nest, ofpact) \
DEFINE_OFPACT(WRITE_METADATA, ofpact_metadata, ofpact) \
DEFINE_OFPACT(GOTO_TABLE, ofpact_goto_table, ofpact)
@@ -384,6 +385,25 @@ struct ofpact_meter {
uint32_t meter_id;
};
+/* OFPACT_WRITE_ACTIONS.
+ *
+ * Used for OFPIT11_WRITE_ACTIONS. */
+struct ofpact_nest {
+ struct ofpact ofpact;
+ uint8_t pad[OFPACT_ALIGN(sizeof(struct ofpact)) - sizeof(struct ofpact)];
+ struct ofpact actions[];
+};
+BUILD_ASSERT_DECL(offsetof(struct ofpact_nest, actions) == OFPACT_ALIGNTO);
+
+static inline size_t
+ofpact_nest_get_action_len(const struct ofpact_nest *on)
+{
+ return on->ofpact.len - offsetof(struct ofpact_nest, actions);
+}
+
+void ofpacts_execute_action_set(struct ofpbuf *action_list,
+ const struct ofpbuf *action_set);
+
/* OFPACT_RESUBMIT.
*
* Used for NXAST_RESUBMIT, NXAST_RESUBMIT_TABLE. */
@@ -665,5 +685,4 @@ enum ovs_instruction_type
ovs_instruction_type_from_ofpact_type(
void ofpact_set_field_init(struct ofpact_reg_load *load,
const struct mf_field *mf, const void *src);
-
#endif /* ofp-actions.h */
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 7ca7305..f55eb3f 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -879,12 +879,11 @@ str_to_ofpact__(char *pos, char *act, char *arg,
* Returns NULL if successful, otherwise a malloc()'d string describing the
* error. The caller is responsible for freeing the returned string. */
static char * WARN_UNUSED_RESULT
-str_to_ofpacts(char *str, struct ofpbuf *ofpacts,
- enum ofputil_protocol *usable_protocols)
+str_to_ofpacts__(char *str, struct ofpbuf *ofpacts,
+ enum ofputil_protocol *usable_protocols)
{
size_t orig_size = ofpacts->size;
char *pos, *act, *arg;
- enum ofperr error;
int n_actions;
pos = str;
@@ -899,13 +898,34 @@ str_to_ofpacts(char *str, struct ofpbuf *ofpacts,
n_actions++;
}
+ ofpact_pad(ofpacts);
+ return NULL;
+}
+
+
+/* Parses 'str' as a series of actions, and appends them to 'ofpacts'.
+ *
+ * Returns NULL if successful, otherwise a malloc()'d string describing the
+ * error. The caller is responsible for freeing the returned string. */
+static char * WARN_UNUSED_RESULT
+str_to_ofpacts(char *str, struct ofpbuf *ofpacts,
+ enum ofputil_protocol *usable_protocols)
+{
+ size_t orig_size = ofpacts->size;
+ char *error_s;
+ enum ofperr error;
+
+ error_s = str_to_ofpacts__(str, ofpacts, usable_protocols);
+ if (error_s) {
+ return error_s;
+ }
+
error = ofpacts_verify(ofpacts->data, ofpacts->size);
if (error) {
ofpacts->size = orig_size;
return xstrdup("Incorrect action ordering");
}
- ofpact_pad(ofpacts);
return NULL;
}
@@ -929,10 +949,22 @@ parse_named_instruction(enum ovs_instruction_type type,
NOT_REACHED(); /* This case is handled by str_to_inst_ofpacts() */
break;
- case OVSINST_OFPIT11_WRITE_ACTIONS:
- /* XXX */
- error_s = xstrdup("instruction write-actions is not supported yet");
+ case OVSINST_OFPIT11_WRITE_ACTIONS: {
+ struct ofpact_nest *on;
+ size_t ofs = ofpacts->size;
+
+ on = ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS,
+ offsetof(struct ofpact_nest, actions));
+ error_s = str_to_ofpacts__(arg, ofpacts, usable_protocols);
+
+ on = ofpbuf_at_assert(ofpacts, ofs, sizeof *on);
+ on->ofpact.len = ofpacts->size - ofs;
+
+ if (error_s) {
+ ofpacts->size = ofs;
+ }
break;
+ }
case OVSINST_OFPIT11_CLEAR_ACTIONS:
ofpact_put_CLEAR_ACTIONS(ofpacts);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index efa5cbe..9313a7f 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -179,6 +179,15 @@ struct xlate_ctx {
odp_port_t sflow_odp_port; /* Output port for composing sFlow action. */
uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
bool exit; /* No further actions should be processed. */
+
+ /* OpenFlow 1.1+ action set.
+ *
+ * 'action_set' accumulates "struct ofpact"s added by OFPACT_WRITE_ACTIONS.
+ * When translation is otherwise complete, ofpacts_execute_action_set()
+ * converts it to a set of "struct ofpact"s that can be translated into
+ * datapath actions. */
+ struct ofpbuf action_set; /* Action set. */
+ uint64_t action_set_stub[1024 / 8];
};
/* A controller may use OFPP_NONE as the ingress port to indicate that
@@ -2247,6 +2256,26 @@ may_receive(const struct xport *xport, struct xlate_ctx
*ctx)
}
static void
+xlate_write_actions(struct xlate_ctx *ctx, const struct ofpact *a)
+{
+ struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
+ ofpbuf_put(&ctx->action_set, on->actions, ofpact_nest_get_action_len(on));
+ ofpact_pad(&ctx->action_set);
+}
+
+static void
+xlate_action_set(struct xlate_ctx *ctx)
+{
+ uint64_t action_list_stub[1024 / 64];
+ struct ofpbuf action_list;
+
+ ofpbuf_use_stub(&action_list, action_list_stub, sizeof action_list_stub);
+ ofpacts_execute_action_set(&action_list, &ctx->action_set);
+ do_xlate_actions(action_list.data, action_list.size, ctx);
+ ofpbuf_uninit(&action_list);
+}
+
+static void
do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
struct xlate_ctx *ctx)
{
@@ -2457,11 +2486,11 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
case OFPACT_CLEAR_ACTIONS:
- /* XXX
- * Nothing to do because writa-actions is not supported for now.
- * When writa-actions is supported, clear-actions also must
- * be supported at the same time.
- */
+ ofpbuf_clear(&ctx->action_set);
+ break;
+
+ case OFPACT_WRITE_ACTIONS:
+ xlate_write_actions(ctx, a);
break;
case OFPACT_WRITE_METADATA:
@@ -2759,6 +2788,8 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
}
ofpbuf_use_stub(&ctx.stack, ctx.init_stack, sizeof ctx.init_stack);
+ ofpbuf_use_stub(&ctx.action_set,
+ ctx.action_set_stub, sizeof ctx.action_set_stub);
if (mbridge_has_mirrors(ctx.xbridge->mbridge)) {
/* Do this conditionally because the copy is expensive enough that it
@@ -2817,6 +2848,10 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
}
}
+ if (ctx.action_set.size) {
+ xlate_action_set(&ctx);
+ }
+
if (ctx.xbridge->has_in_band
&& in_band_must_output_to_local_port(flow)
&& !actions_output_to_local_port(&ctx)) {
@@ -2840,6 +2875,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
}
ofpbuf_uninit(&ctx.stack);
+ ofpbuf_uninit(&ctx.action_set);
/* Clear the metadata and register wildcard masks, because we won't
* use non-header fields as part of the cache. */
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index ebad040..0909ce1 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -416,9 +416,29 @@ dnl and OVS reorders it to the canonical order)
# 31: ff -> 00
0001 0008 01 000000 0002 0018 00000000 fedcba9876543210 ffffffffffffffff
-dnl Write-Actions not supported yet.
-# bad OF1.1 instructions: OFPBIC_UNSUP_INST
-0003 0008 01 000000
+dnl empty Write-Actions non-zero padding
+# actions=write_actions(drop)
+# 0: 00 -> (none)
+# 1: 03 -> (none)
+# 2: 00 -> (none)
+# 3: 08 -> (none)
+# 4: 00 -> (none)
+# 5: 00 -> (none)
+# 6: 00 -> (none)
+# 7: 01 -> (none)
+0003 0008 00000001
+
+dnl Check that an empty Write-Actions instruction gets dropped.
+# actions=write_actions(drop)
+# 0: 00 -> (none)
+# 1: 03 -> (none)
+# 2: 00 -> (none)
+# 3: 08 -> (none)
+# 4: 00 -> (none)
+# 5: 00 -> (none)
+# 6: 00 -> (none)
+# 7: 00 -> (none)
+0003 0008 00000000
dnl Clear-Actions too-long
# bad OF1.1 instructions: OFPBIC_BAD_LEN
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 5316ce9..3630cda 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -34,6 +34,37 @@ AT_CHECK([tail -1 stdout], [0],
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - write actions])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [10], [11], [12], [13])
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1,ip
actions=output(10),write_actions(set_field:192.168.3.90->ip_src,output(12)),goto_table(1)
+table=1 ip actions=write_actions(output(13)),goto_table(2)
+table=2 ip actions=set_field:192.168.3.91->ip_src,output(11)
+])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt])
+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:
10,set(ipv4(src=192.168.3.91,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no)),11,set(ipv4(src=192.168.3.90,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no)),13
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - clear actions])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [10], [11], [12])
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1,ip
actions=output(10),write_actions(set_field:192.168.3.90->ip_src,output(12)),goto_table(1)
+table=1 ip actions=set_field:192.168.3.91->ip_src,output(11),clear_actions
+])
+AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt])
+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:
10,set(ipv4(src=192.168.3.91,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no)),11
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - registers])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [20], [21], [22], [33], [90])
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index c43b48c..b2bc8d7 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1346,6 +1346,87 @@ to \fBactions=\fR field.
.IP \fBclear_actions\fR
Clears all the actions in the action set immediately.
.
+.IP \fBwrite_actions(\fR[\fIaction\fR][\fB,\fIaction\fR...]\fB)
+Add the specific actions to the action set. The syntax of
+\fIactions\fR is the same as in the \fBactions=\fR field. The action
+set is carried between flow tables and then executed at the end of the
+pipeline.
+.
+.IP
+The actions in the action set are applied in the following order, as
+required by the OpenFlow specification, regardless of the order in
+which they were added to the action set. Except as specified
+otherwise below, the action set only holds at most a single action of
+each type. When more than one action of a single type is written to
+the action set, the one written later replaces the earlier action:
+.
+.RS
+.IP 1.
+\fBstrip_vlan\fR
+.IQ
+\fBpop_mpls\fR
+.
+.IP 2.
+\fBpush_mpls\fR
+.
+.IP 3.
+\fBpush_vlan\fR
+.
+.IP 4.
+\fBdec_ttl\fR
+.IQ
+\fBdec_mpls_ttl\fR
+.
+.IP 5.
+\fBload\fR
+.IQ
+\fBmod_dl_dst\fR
+.IQ
+\fBmod_dl_src\fR
+.IQ
+\fBmod_nw_dst\fR
+.IQ
+\fBmod_nw_src\fR
+.IQ
+\fBmod_nw_tos\fR
+.IQ
+\fBmod_tp_dst\fR
+.IQ
+\fBmod_tp_src\fR
+.IQ
+\fBmod_vlan_pcp\fR
+.IQ
+\fBmod_vlan_vid\fR
+.IQ
+\fBset_field\fR
+.IQ
+\fBset_tunnel\fR
+.IQ
+\fBset_tunnel64\fR
+.IQ
+The action set can contain any number of these actions, with
+cumulative effect. That is, when multiple actions modify the same
+part of a field, the later modification takes effect, and when they
+modify different parts of a field (or different fields), then both
+modifications are applied.
+.
+.IP 6.
+\fBset_queue\fR
+.
+.IP 7.
+\fBgroup\fR
+.IQ
+\fBoutput\fR
+.IQ
+If both actions are present, then \fBgroup\fR is executed and
+\fBoutput\fR is ignored, regardless of the order in which they were
+added to the action set. (If neither action is present, the action
+set has no real effect, because the modified packet is not sent
+anywhere and thus the modifications are not visible.)
+.RE
+.IP
+Only the actions listed above may be written to the action set.
+.
.IP \fBwrite_metadata\fB:\fIvalue\fR[/\fImask\fR]
Updates the metadata field for the flow. If \fImask\fR is omitted, the
metadata field is set exactly to \fIvalue\fR; if \fImask\fR is specified, then
@@ -1409,10 +1490,12 @@ configuring sample collector sets.
This action was added in Open vSwitch 1.10.90.
.
.IP "\fBexit\fR"
-This action causes Open vSwitch to immediately halt execution of further
-actions. Those actions which have already been executed are unaffected. Any
-further actions, including those which may be in other tables, or different
-levels of the \fBresubmit\fR call stack, are ignored.
+This action causes Open vSwitch to immediately halt execution of
+further actions. Those actions which have already been executed are
+unaffected. Any further actions, including those which may be in
+other tables, or different levels of the \fBresubmit\fR call stack,
+are ignored. Actions in the action set is still executed (specify
+\fBclear_actions\fR before \fBexit\fR to discard them).
.RE
.
.PP
--
1.7.10.4
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
