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.

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 4000d339ad2e73d61407bb92487f0be1b5889fa4..23ca528ce8fd1d6f5131a376aa5a3da7be55dd98 100644 (file)
--- 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 a362a65a1a4e59fae5fe0a3cfe028e54df1b7b96..85030560410ace2bb18f2d1e3a6ac47e24aa941e 100644 (file)
--- 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 2d10fcbfb82c2e416327b1c16bcda83c54fd3a57..56b1e41254cc539d208703a421a7cde8d53a100a 100644 (file)
--- 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 baf25e53290fed15d9ee7b264b62ce86cc3074ed..8fb5b1bc15fa530127cd6f08398ae9b36014e347 100644 (file)
--- 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 c3ef8f7575695136f55ea4f185cd10b053bc3e8b..5ce05d6e4c0c5ddc1486e1c852c874151ce8356d 100644 (file)
--- 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 64adef669b11693c55b3434e717f4b65b0cd9a37..efa3686e04503330c533f797289536340fb49296 100644 (file)
--- 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 3d101789df92fb79499baa455f269ba7b5aaef90..a6fff256c71f35ba02f8cf92f0d37fd39d1ec3ac 100644 (file)
--- 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);
 }