[ovs-dev] [PATCH v6 1/4] Add support for write-actions

Simon Horman horms at verge.net.au
Fri Oct 11 04:23:29 UTC 2013


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 <horms at verge.net.au>

---

v6
* No change

v5
* Add OFPACT_DEC_MPLS_TTL to ofpacts_list_to_action_set()
  This was left out of the OFPACT_DEC_MPLS_TTL update made
  in v4.

v4
* Rebase
* As suggested by Jarno Rajahalme
  - Group instructions in ofpact_is_allowed_in_actions_set()
    and add comment about why they are exclude from the actions set.
  - Allow OFPACT_DEC_MPLS_TTL in action set.
  - Add note about why some non-OpenFlow actions are excluded from
    the action set

v3
* No change

v2
* First post
---
 lib/ofp-actions.c            | 354 +++++++++++++++++++++++++++++++++++++++++--
 lib/ofp-actions.h            |  22 ++-
 lib/ofp-parse.c              |  43 +++++-
 ofproto/ofproto-dpif-xlate.c |  67 +++++++-
 tests/ofp-actions.at         |  26 +++-
 tests/ofproto-dpif.at        |  27 ++++
 utilities/ovs-ofctl.8.in     |  51 +++++++
 7 files changed, 559 insertions(+), 31 deletions(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 65430f3..ae1ccf1 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -880,6 +880,262 @@ 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));
+}
+
+/* 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,
+                  enum ofpact_type filter)
+{
+    const struct ofpact *a;
+
+    OFPACT_FOR_EACH (a, in, in_len) {
+        if (a->type == filter) {
+            return 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;
+        }
+    }
+
+    return false;
+}
+
+/* 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))
+{
+    const struct ofpact *a;
+
+    OFPACT_FOR_EACH (a, in, in_len) {
+        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.
+ * 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_list_to_action_set(struct ofpbuf *out, const struct list *in)
+{
+    /* The order here is important.
+     * It corresponds to the order set out for the action set
+     * in the OpenFlow specification.  */
+
+    /* 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);
+    }
+}
+
+
+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 +1202,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 +1428,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 +1466,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) {
@@ -1205,6 +1474,10 @@ 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,
@@ -1292,6 +1565,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;
 
@@ -1622,6 +1903,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 +1998,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 +2175,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 +2300,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 +2363,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:
@@ -2195,7 +2491,11 @@ print_fin_timeout(const struct ofpact_fin_timeout *fin_timeout,
 }
 
 static void
-ofpact_format(const struct ofpact *a, struct ds *s)
+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)
 {
     const struct ofpact_enqueue *enqueue;
     const struct ofpact_resubmit *resubmit;
@@ -2416,6 +2716,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, loop);
+        ds_put_char(s, ')');
+        break;
+    }
+
     case OFPACT_CLEAR_ACTIONS:
         ds_put_format(s, "%s",
                       ovs_instruction_name_from_type(
@@ -2453,13 +2763,19 @@ ofpact_format(const struct ofpact *a, struct ds *s)
     }
 }
 
+#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'. */
-void
-ofpacts_format(const struct ofpact *ofpacts, size_t ofpacts_len,
-               struct ds *string)
+static void
+ofpacts_format__(const struct ofpact *ofpacts, size_t ofpacts_len,
+                 struct ds *string, int loop)
 {
-    ds_put_cstr(string, "actions=");
+    if (loop++ > OFPACTS_FORMAT_RECURSION_LIMIT) {
+        NOT_REACHED();
+    }
+
     if (!ofpacts_len) {
         ds_put_cstr(string, "drop");
     } else {
@@ -2471,10 +2787,20 @@ ofpacts_format(const struct ofpact *ofpacts, size_t ofpacts_len,
             }
 
             /* XXX write-actions */
-            ofpact_format(a, string);
+            ofpact_format(a, string, loop);
         }
     }
 }
+
+/* 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 0876ed7..129d6e8 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -99,8 +99,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 +384,15 @@ 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[];
+};
+
 /* OFPACT_RESUBMIT.
  *
  * Used for NXAST_RESUBMIT, NXAST_RESUBMIT_TABLE. */
@@ -499,6 +508,16 @@ 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,
@@ -665,5 +684,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..e9b97e9 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,19 @@ 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 orig_size = ofpacts->size;
+
+        on = ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS,
+                         offsetof(struct ofpact_nest, actions));
+        error_s = str_to_ofpacts__(arg, ofpacts, usable_protocols);
+        on->ofpact.len = ofpacts->size - orig_size;
+        if (error_s) {
+            ofpacts->size = orig_size;
+        }
         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 4fb0d5e..82bb664 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -179,6 +179,10 @@ 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. */
+
+    struct list action_set;     /* Action set.
+                                 * An ordered list of struct ofpbuf
+                                 * containing ofpacts to add to the action. */
 };
 
 /* A controller may use OFPP_NONE as the ingress port to indicate that
@@ -2247,6 +2251,53 @@ 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));
+}
+
+static void
+xlate_action_set(struct xlate_ctx *ctx)
+{
+    struct ofpbuf ofpact_buf;
+
+    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);
+}
+
+static void
 do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
                  struct xlate_ctx *ctx)
 {
@@ -2457,11 +2508,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.
-             */
+            action_set_clear(ctx);
+            break;
+
+        case OFPACT_WRITE_ACTIONS:
+            xlate_write_actions(ctx, a);
             break;
 
         case OFPACT_WRITE_METADATA:
@@ -2481,6 +2532,9 @@ 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;
         }
 
@@ -2489,6 +2543,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
         }
     }
+
+    xlate_action_set(ctx);
 }
 
 void
@@ -2737,6 +2793,7 @@ 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);
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..cee6f2d 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -34,6 +34,33 @@ 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])
+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_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])
+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_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..400f10a 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1346,6 +1346,57 @@ 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)
+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.
+.
+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
+\fBpush_mpls\fR
+.IP
+\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
+\fBset_queue\fR
+.IP
+\fBgroup\fR
+.IP
+\fBoutput\fR
+Not added if a group action is present in 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
-- 
1.8.4




More information about the dev mailing list