ofp-util: Simplify iteration through OpenFlow actions.

The existing actions_first() and actions_next() iterator functions are not
much like the other iteration constructs found throughout the Open vSwitch
tree.  Also, they only work with actions that have already been validated,
so there are cases where they cannot be used.

This commit adds new macros for iterating through OpenFlow actions, one
for actions that have been validated and one for actions that have not, and
adapts the existing users.  The following commit will further refine action
parsing and add more users.

7 files changed, 66 insertions, 95 deletions

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 4000d339..23ca528c 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -332,43 +332,21 @@ ofp_action_len(enum ofp_action_type type)
     }
 }
 
-static int
-ofp_print_action(struct ds *string, const struct ofp_action_header *ah,
-        size_t actions_len)
+static void
+ofp_print_action(struct ds *string, const struct ofp_action_header *ah)
 {
     enum ofp_action_type type;
     int required_len;
     size_t len;
 
-    if (actions_len < sizeof *ah) {
-        ds_put_format(string, "***action array too short for next action***\n");
-        return -1;
-    }
-
     type = ntohs(ah->type);
     len = ntohs(ah->len);
-    if (actions_len < len) {
-        ds_put_format(string, "***truncated action %d***\n", (int) type);
-        return -1;
-    }
-
-    if (!len) {
-        ds_put_format(string, "***zero-length action***\n");
-        return 8;
-    }
-
-    if ((len % OFP_ACTION_ALIGN) != 0) {
-        ds_put_format(string,
-                      "***action %d length not a multiple of %d***\n",
-                      (int) type, OFP_ACTION_ALIGN);
-        return -1;
-    }
 
     required_len = ofp_action_len(type);
     if (required_len >= 0 && len != required_len) {
         ds_put_format(string,
                       "***action %d wrong length: %zu***\n", (int) type, len);
-        return -1;
+        return;
     }
 
     switch (type) {
@@ -469,7 +447,7 @@ ofp_print_action(struct ds *string, const struct ofp_action_header *ah,
                 = (struct ofp_action_vendor_header *)ah;
         if (len < sizeof *avh) {
             ds_put_format(string, "***ofpat_vendor truncated***\n");
-            return -1;
+            return;
         }
         if (avh->vendor == htonl(NX_VENDOR_ID)) {
             ofp_print_nx_action(string, (struct nx_action_header *)avh);
@@ -483,32 +461,28 @@ ofp_print_action(struct ds *string, const struct ofp_action_header *ah,
         ds_put_format(string, "(decoder %d not implemented)", (int) type);
         break;
     }
-
-    return len;
 }
 
 void
-ofp_print_actions(struct ds *string, const struct ofp_action_header *action,
-                  size_t actions_len)
+ofp_print_actions(struct ds *string, const union ofp_action *actions,
+                  size_t n_actions)
 {
-    uint8_t *p = (uint8_t *)action;
-    int len = 0;
+    const union ofp_action *a;
+    size_t left;
 
     ds_put_cstr(string, "actions=");
-    if (!actions_len) {
+    if (!n_actions) {
         ds_put_cstr(string, "drop");
     }
-    while (actions_len > 0) {
-        if (len) {
+    OFPUTIL_ACTION_FOR_EACH (a, left, actions, n_actions) {
+        if (a != actions) {
             ds_put_cstr(string, ",");
         }
-        len = ofp_print_action(string, (struct ofp_action_header *)p,
-                actions_len);
-        if (len < 0) {
-            return;
-        }
-        p += len;
-        actions_len -= len;
+        ofp_print_action(string, (struct ofp_action_header *)a);
+    }
+    if (left > 0) {
+        ds_put_format(string, " ***%zu leftover bytes following actions",
+                      left * sizeof *a);
     }
 }
 
@@ -527,7 +501,12 @@ ofp_print_packet_out(struct ds *string, const struct ofp_packet_out *opo,
         ds_put_format(string, "***packet too short for action length***\n");
         return;
     }
-    ofp_print_actions(string, opo->actions, actions_len);
+    if (actions_len % sizeof(union ofp_action)) {
+        ds_put_format(string, "***action length not a multiple of %zu***\n",
+                      sizeof(union ofp_action));
+    }
+    ofp_print_actions(string, (const union ofp_action *) opo->actions,
+                      actions_len / sizeof(union ofp_action));
 
     if (ntohl(opo->buffer_id) == UINT32_MAX) {
         int data_len = len - sizeof *opo - actions_len;
@@ -907,8 +886,7 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh,
         ds_put_format(s, "flags:0x%"PRIx16" ", fm.flags);
     }
 
-    ofp_print_actions(s, (const struct ofp_action_header *) fm.actions,
-                      fm.n_actions * sizeof *fm.actions);
+    ofp_print_actions(s, fm.actions, fm.n_actions);
 }
 
 static void
@@ -1123,9 +1101,7 @@ ofp_print_flow_stats_reply(struct ds *string, const struct ofp_header *oh)
 
         cls_rule_format(&fs.rule, string);
         ds_put_char(string, ' ');
-        ofp_print_actions(string,
-                          (const struct ofp_action_header *) fs.actions,
-                          fs.n_actions * sizeof *fs.actions);
+        ofp_print_actions(string, fs.actions, fs.n_actions);
      }
 }
 
diff --git a/lib/ofp-print.h b/lib/ofp-print.h
index a362a65a..85030560 100644
--- a/lib/ofp-print.h
+++ b/lib/ofp-print.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009 Nicira Networks.
+ * Copyright (c) 2008, 2009, 2011 Nicira Networks.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -25,7 +25,7 @@
 struct ofp_flow_mod;
 struct ofp_match;
 struct ds;
-struct ofp_action_header;
+union ofp_action;
 
 #ifdef  __cplusplus
 extern "C" {
@@ -34,7 +34,7 @@ extern "C" {
 void ofp_print(FILE *, const void *, size_t, int verbosity);
 void ofp_print_packet(FILE *stream, const void *data, size_t len, size_t total_len);
 
-void ofp_print_actions(struct ds *, const struct ofp_action_header *, size_t);
+void ofp_print_actions(struct ds *, const union ofp_action *, size_t);
 void ofp_print_match(struct ds *, const struct ofp_match *, int verbosity);
 
 char *ofp_to_string(const void *, size_t, int verbosity);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 2d10fcbf..56b1e412 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -2206,30 +2206,6 @@ action_outputs_to_port(const union ofp_action *action, ovs_be16 port)
     }
 }
 
-/* The set of actions must either come from a trusted source or have been
- * previously validated with validate_actions(). */
-const union ofp_action *
-actions_first(struct actions_iterator *iter,
-              const union ofp_action *oa, size_t n_actions)
-{
-    iter->pos = oa;
-    iter->end = oa + n_actions;
-    return actions_next(iter);
-}
-
-const union ofp_action *
-actions_next(struct actions_iterator *iter)
-{
-    if (iter->pos != iter->end) {
-        const union ofp_action *a = iter->pos;
-        unsigned int len = ntohs(a->header.len);
-        iter->pos += len / OFP_ACTION_ALIGN;
-        return a;
-    } else {
-        return NULL;
-    }
-}
-
 /* "Normalizes" the wildcards in 'rule'.  That means:
  *
  *    1. If the type of level N is known, then only the valid fields for that
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index baf25e53..8fb5b1bc 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -277,13 +277,36 @@ struct ofpbuf *make_echo_reply(const struct ofp_header *rq);
 
 #define OFP_ACTION_ALIGN 8      /* Alignment of ofp_actions. */
 
-struct actions_iterator {
-    const union ofp_action *pos, *end;
-};
-const union ofp_action *actions_first(struct actions_iterator *,
-                                      const union ofp_action *,
-                                      size_t n_actions);
-const union ofp_action *actions_next(struct actions_iterator *);
+static inline union ofp_action *
+ofputil_action_next(const union ofp_action *a)
+{
+    return (void *) ((uint8_t *) a + ntohs(a->header.len));
+}
+
+static inline bool
+ofputil_action_is_valid(const union ofp_action *a, size_t n_actions)
+{
+    uint16_t len = ntohs(a->header.len);
+    return (!(len % OFP_ACTION_ALIGN)
+            && len >= sizeof *a
+            && len / sizeof *a <= n_actions);
+}
+
+/* This macro is careful to check for actions with bad lengths. */
+#define OFPUTIL_ACTION_FOR_EACH(ITER, LEFT, ACTIONS, N_ACTIONS)         \
+    for ((ITER) = (ACTIONS), (LEFT) = (N_ACTIONS);                      \
+         (LEFT) > 0 && ofputil_action_is_valid(ITER, LEFT);             \
+         ((LEFT) -= ntohs((ITER)->header.len) / sizeof(union ofp_action), \
+          (ITER) = ofputil_action_next(ITER)))
+
+/* This macro does not check for actions with bad lengths.  It should only be
+ * used with actions from trusted sources or with actions that have already
+ * been validated (e.g. with OFPUTIL_ACTION_FOR_EACH).  */
+#define OFPUTIL_ACTION_FOR_EACH_UNSAFE(ITER, LEFT, ACTIONS, N_ACTIONS)  \
+    for ((ITER) = (ACTIONS), (LEFT) = (N_ACTIONS);                      \
+         (LEFT) > 0;                                                    \
+         ((LEFT) -= ntohs((ITER)->header.len) / sizeof(union ofp_action), \
+          (ITER) = ofputil_action_next(ITER)))
 
 int validate_actions(const union ofp_action *, size_t n_actions,
                      const struct flow *, int max_ports);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c3ef8f75..5ce05d6e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3115,8 +3115,8 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
                  struct action_xlate_ctx *ctx)
 {
     const struct ofport_dpif *port;
-    struct actions_iterator iter;
     const union ofp_action *ia;
+    size_t left;
 
     port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
     if (port
@@ -3128,9 +3128,9 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
         return;
     }
 
-    for (ia = actions_first(&iter, in, n_in); ia; ia = actions_next(&iter)) {
-        enum ofp_action_type type = ntohs(ia->type);
+    OFPUTIL_ACTION_FOR_EACH_UNSAFE (ia, left, in, n_in) {
         const struct ofp_action_dl_addr *oada;
+        enum ofp_action_type type = ntohs(ia->type);
 
         switch (type) {
         case OFPAT_OUTPUT:
@@ -3852,8 +3852,7 @@ trace_format_rule(struct ds *result, int level, const struct rule *rule)
 
     ds_put_char_multiple(result, '\t', level);
     ds_put_cstr(result, "OpenFlow ");
-    ofp_print_actions(result, (const struct ofp_action_header *) rule->actions,
-                      rule->n_actions * sizeof *rule->actions);
+    ofp_print_actions(result, rule->actions, rule->n_actions);
     ds_put_char(result, '\n');
 }
 
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 64adef66..efa3686e 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1398,13 +1398,12 @@ static bool
 rule_has_out_port(const struct rule *rule, uint16_t out_port)
 {
     const union ofp_action *oa;
-    struct actions_iterator i;
+    size_t left;
 
     if (out_port == OFPP_NONE) {
         return true;
     }
-    for (oa = actions_first(&i, rule->actions, rule->n_actions); oa;
-         oa = actions_next(&i)) {
+    OFPUTIL_ACTION_FOR_EACH_UNSAFE (oa, left, rule->actions, rule->n_actions) {
         if (action_outputs_to_port(oa, htons(out_port))) {
             return true;
         }
@@ -1921,7 +1920,6 @@ static void
 flow_stats_ds(struct rule *rule, struct ds *results)
 {
     uint64_t packet_count, byte_count;
-    size_t act_len = sizeof *rule->actions * rule->n_actions;
 
     rule->ofproto->ofproto_class->rule_get_stats(rule,
                                                  &packet_count, &byte_count);
@@ -1936,8 +1934,8 @@ flow_stats_ds(struct rule *rule, struct ds *results)
     ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count);
     cls_rule_format(&rule->cr, results);
     ds_put_char(results, ',');
-    if (act_len > 0) {
-        ofp_print_actions(results, &rule->actions->header, act_len);
+    if (rule->n_actions > 0) {
+        ofp_print_actions(results, rule->actions, rule->n_actions);
     } else {
         ds_put_cstr(results, "drop");
     }
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 3d101789..a6fff256 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -951,8 +951,7 @@ fte_version_print(const struct fte_version *version)
     }
 
     ds_init(&s);
-    ofp_print_actions(&s, (const struct ofp_action_header *) version->actions,
-                      version->n_actions * sizeof *version->actions);
+    ofp_print_actions(&s, version->actions, version->n_actions);
     printf(" %s\n", ds_cstr(&s));
     ds_destroy(&s);
 }