[ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation

[Ben Pfaff]

On Mon, Oct 30, 2017 at 12:22:50PM +0100, Zoltan Balogh wrote:
> When all OF actions have been translated, there could be actions at
> the end of list of odp actions which are not needed to be executed.
> So, the list can be normalized at the end of xlate_actions().
> 
> Signed-off-by: Zoltan Balogh <zoltan.balogh at ericsson.com>
> Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>
> Co-authored-by: Sugesh Chandran <sugesh.chandran at intel.com>
> Tested-by: Sugesh Chandran <sugesh.chandran at intel.com>

Thanks for working on this!  It will be helpful in some cases.

In is_valid_last_action(), I recommend assigning nl_attr_type(nla) to a
variable of type enum ovs_action_attr, then using that for the switch
statement.  That will ensure that, as new kinds of actions are added, we
remember to add new items to the switch statement.

Here are some minor simplifications that you might consider folding in
also.  They compile, but I have not tested them.

--8<--------------------------cut here-------------------------->8--

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5e400e091ac6..d08cfddc55cb 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6943,7 +6943,7 @@ xlate_wc_finish(struct xlate_ctx *ctx)
 /* Returns true if the action stored in 'nla' can be a valid last action of a
  * datapath flow. */
 static bool
-is_valid_last_action(struct nlattr *nla)
+is_valid_last_action(const struct nlattr *nla)
 {
     switch (nl_attr_type(nla)) {
     case OVS_ACTION_ATTR_USERSPACE:
@@ -6965,35 +6965,27 @@ is_valid_last_action(struct nlattr *nla)
  * 'data'. Execution of actions beyond this last attribute does not make sense.
  */
 static size_t
-last_action_offset(struct nlattr *data, const size_t data_len)
+last_action_offset(const struct nlattr *data, const size_t data_len)
 {
-    uint16_t left;
-    struct nlattr *a, *b = NULL;
+    const struct nlattr *last = data;
 
+    uint16_t left;
+    const struct nlattr *a;
     NL_ATTR_FOR_EACH (a, left, data, data_len) {
         if (is_valid_last_action(a)) {
-            b = a;
+            last = nl_attr_next(a);
         }
     }
 
-    if (b) {
-        return NLA_ALIGN(((char *)b - (char *)data) + b->nla_len);
-    } else {
-        return 0;
-    }
+    return (char *) last - (char *) data;
 }
 
+/* Get rid of any unneeded actions at the tail end. */
 static void
 normalize_odp_actions(struct xlate_ctx *ctx)
 {
-    struct nlattr *data = ctx->odp_actions->data;
-    size_t size = ctx->odp_actions->size;
-    size_t new_size = last_action_offset(data, size);
-
-    /* Get rid of any unneeded actions at the tail end. */
-    if (OVS_UNLIKELY(new_size != size)) {
-        ctx->odp_actions->size = new_size;
-    }
+    struct ofpbuf *oa = ctx->odp_actions;
+    oa->size = last_action_offset(oa->data, oa->size);
 }
 
 /* Translates the flow, actions, or rule in 'xin' into datapath actions in