From: Ben Pfaff <blp@nicira.com>
Date: Thu, 17 Nov 2011 18:24:05 +0000 (-0800)
Subject: tests: Rewrite code for comparing sets of ODP actions.
X-Git-Tag: v1.4.0~134
X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=becffb862616fa1a5c76c948b4f39cd61f6efca8;p=sliver-openvswitch.git

tests: Rewrite code for comparing sets of ODP actions.

The compare-odp-actions.pl utility isn't fully general, even for its
intended purpose of allowing sets of ODP actions to be compared
ignoring unimportant differences in ordering of output actions and
VLAN set actions.  I decided that the proper way to do it was to have
a utility that can actually parse the actions, instead of just
doing textual transformations on them.  So, this commit replaces
compare-odp-actions.pl by "ovs-dpctl normalize-actions", which is
sufficiently general for the intended purpose.

The new ovs-dpctl functionality can be easily extended to handle
differences in fields other than VLAN, but only VLAN is needed so
far.

This will be needed in an upcoming commit that in some cases
introduces redundant "set vlan" actions into the ODP actions, which
compare-odp-actions.pl doesn't tolerate.
---

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 935633f31..28dd537f0 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -116,21 +116,6 @@ ovs_key_attr_to_string(enum ovs_key_attr attr)
     }
 }
 
-static enum ovs_key_attr
-ovs_key_attr_from_string(const char *s, size_t len)
-{
-    enum ovs_key_attr attr;
-
-    for (attr = 0; attr <= OVS_KEY_ATTR_MAX; attr++) {
-        const char *attr_name = ovs_key_attr_to_string(attr);
-        if (strlen(attr_name) == len && !memcmp(s, attr_name, len)) {
-            return attr;
-        }
-    }
-
-    return OVS_KEY_ATTR_UNSPEC;
-}
-
 static void
 format_generic_odp_action(struct ds *ds, const struct nlattr *a)
 {
@@ -409,33 +394,35 @@ parse_odp_action(const char *s, const struct shash *port_names,
         return retval + 5;
     }
 
-    if (!strncmp(s, "push(", 5)) {
-        size_t start_ofs;
-        int retval;
+    {
+        struct ovs_action_push_vlan push;
+        int tpid = ETH_TYPE_VLAN;
+        int vid, pcp;
+        int cfi = 1;
+        int n = -1;
 
-        start_ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_PUSH);
-        retval = parse_odp_key_attr(s + 5, port_names, actions);
-        if (retval < 0) {
-            return retval;
-        }
-        if (s[retval + 5] != ')') {
-            return -EINVAL;
+        if ((sscanf(s, "push_vlan(vid=%i,pcp=%i)%n", &vid, &pcp, &n) > 0
+             && n > 0)
+            || (sscanf(s, "push_vlan(vid=%i,pcp=%i,cfi=%i)%n",
+                       &vid, &pcp, &cfi, &n) > 0 && n > 0)
+            || (sscanf(s, "push_vlan(tpid=%i,vid=%i,pcp=%i)%n",
+                       &tpid, &vid, &pcp, &n) > 0 && n > 0)
+            || (sscanf(s, "push_vlan(tpid=%i,vid=%i,pcp=%i,cfi=%i)%n",
+                       &tpid, &vid, &pcp, &cfi, &n) > 0 && n > 0)) {
+            push.vlan_tpid = htons(tpid);
+            push.vlan_tci = htons((vid << VLAN_VID_SHIFT)
+                                  | (pcp << VLAN_PCP_SHIFT)
+                                  | (cfi ? VLAN_CFI : 0));
+            nl_msg_put_unspec(actions, OVS_ACTION_ATTR_PUSH_VLAN,
+                              &push, sizeof push);
+
+            return n;
         }
-        nl_msg_end_nested(actions, start_ofs);
-        return retval + 6;
     }
 
-    if (!strncmp(s, "pop(", 4)) {
-        enum ovs_key_attr key;
-        size_t len;
-
-        len = strcspn(s + 4, ")");
-        key = ovs_key_attr_from_string(s + 4, len);
-        if (key == OVS_KEY_ATTR_UNSPEC || s[4 + len] != ')') {
-            return -EINVAL;
-        }
-        nl_msg_put_u16(actions, OVS_ACTION_ATTR_POP, key);
-        return len + 5;
+    if (!strncmp(s, "pop_vlan", 8)) {
+        nl_msg_put_flag(actions, OVS_ACTION_ATTR_POP_VLAN);
+        return 8;
     }
 
     {
@@ -1107,7 +1094,7 @@ parse_odp_key_attr(const char *s, const struct shash *port_names,
                 break;
             }
 
-            retval = parse_odp_key_attr(s, key);
+            retval = parse_odp_key_attr(s, port_names, key);
             if (retval < 0) {
                 return retval;
             }
diff --git a/tests/automake.mk b/tests/automake.mk
index f11e2918c..09de52152 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -58,7 +58,6 @@ TESTSUITE_AT = \
 	tests/vlog.at
 TESTSUITE = $(srcdir)/tests/testsuite
 DISTCLEANFILES += tests/atconfig tests/atlocal
-EXTRA_DIST += tests/compare-odp-actions.pl
 
 AUTOTEST_PATH = utilities:vswitchd:ovsdb:tests
 
diff --git a/tests/compare-odp-actions.pl b/tests/compare-odp-actions.pl
deleted file mode 100644
index b18a593c7..000000000
--- a/tests/compare-odp-actions.pl
+++ /dev/null
@@ -1,66 +0,0 @@
-# -*- perl -*-
-
-use strict;
-use warnings;
-
-if (@ARGV < 2) {
-    print <<EOF;
-$0: to check ODP sets of actions for equivalence
-usage: $0 ACTIONS1 ACTIONS2 [NAME=NUMBER]...
-where ACTIONS1 and ACTIONS2 are sets of ODP actions as output by, e.g.
-      "ovs-dpctl dump-flows" and each NAME=NUMBER pair identifies an ODP
-      port's name-to-number mapping.
-
-Exits with status 0 if ACTIONS1 and ACTIONS2 are equivalent, with
-status 1 if they differ.
-EOF
-    exit 1;
-}
-
-# Construct mappings between port numbers and names.
-our (%name_to_number);
-our (%number_to_name);
-for (@ARGV[2..$#ARGV]) {
-    my ($name, $number) = /([^=]+)=([0-9]+)/
-      or die "$_: bad syntax (use --help for help)\n";
-    $number_to_name{$number} = $name;
-    $name_to_number{$name} = $number;
-}
-
-my $n1 = normalize_odp_actions($ARGV[0]);
-my $n2 = normalize_odp_actions($ARGV[1]);
-print "Normalized action set 1: $n1\n";
-print "Normalized action set 2: $n2\n";
-exit($n1 ne $n2);
-
-sub normalize_odp_actions {
-    my ($actions) = @_;
-
-    # Transliterate all commas inside parentheses into semicolons.
-    undef while $actions =~ s/(\([^),]*),([^)]*\))/$1;$2/g;
-
-    # Split on commas.
-    my (@actions) = split(',', $actions);
-
-    # Map port numbers into port names.
-    foreach my $s (@actions) {
-        $s = $number_to_name{$s} if exists($number_to_name{$s});
-    }
-
-    # Sort sequential groups of port names into alphabetical order.
-    for (my $i = 0; $i <= $#actions; ) {
-        my $j = $i + 1;
-        if (exists($name_to_number{$actions[$i]})) {
-            for (; $j <= $#actions; $j++) {
-                last if !exists($name_to_number{$actions[$j]});
-            }
-        }
-        @actions[$i..($j - 1)] = sort(@actions[$i..($j - 1)]);
-        $i = $j;
-    }
-
-    # Now compose a string again and transliterate semicolons back to commas.
-    $actions = join(',', @actions);
-    $actions =~ tr/;/,/;
-    return $actions;
-}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index f3f1738fe..5aec52211 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -220,8 +220,10 @@ do
   AT_CHECK([ovs-appctl ofproto/trace br0 "$flow"], [0], [stdout])
   actual=`tail -1 stdout | sed 's/Datapath actions: //'`
 
-  AT_CHECK([echo "in_port=$in_port vlan=$vlan"
-            $PERL $srcdir/compare-odp-actions.pl "$expected" "$actual" br0=$br0 p1=$p1 p2=$p2 p3=$p3 p4=$p4 p5=$p5 p6=$p6 p7=$p7 p8=$p8], [0], [ignore])
+  echo "in_port=$in_port vlan=$vlan"
+  AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected" br0=$br0 p1=$p1 p2=$p2 p3=$p3 p4=$p4 p5=$p5 p6=$p6 p7=$p7 p8=$p8], [0], [stdout])
+  mv stdout expout
+  AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual" br0=$br0 p1=$p1 p2=$p2 p3=$p3 p4=$p4 p5=$p5 p6=$p6 p7=$p7 p8=$p8], [0], [expout])
 done
 
 OVS_VSWITCHD_STOP
diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 69a66b69a..5fa6fb7eb 100644
--- a/utilities/ovs-dpctl.c
+++ b/utilities/ovs-dpctl.c
@@ -16,6 +16,7 @@
 
 #include <config.h>
 #include <arpa/inet.h>
+#include <assert.h>
 #include <errno.h>
 #include <getopt.h>
 #include <inttypes.h>
@@ -35,9 +36,12 @@
 #include "dirs.h"
 #include "dpif.h"
 #include "dynamic-string.h"
+#include "flow.h"
 #include "netdev.h"
+#include "netlink.h"
 #include "odp-util.h"
 #include "ofpbuf.h"
+#include "packets.h"
 #include "shash.h"
 #include "sset.h"
 #include "timeval.h"
@@ -49,6 +53,12 @@ VLOG_DEFINE_THIS_MODULE(dpctl);
 /* -s, --statistics: Print port statistics? */
 static bool print_statistics;
 
+/* -m, --more: Output verbosity.
+ *
+ * So far only undocumented commands honor this option, so we don't document
+ * the option itself. */
+static int verbosity;
+
 static const struct command all_commands[];
 
 static void usage(void) NO_RETURN;
@@ -73,6 +83,7 @@ parse_options(int argc, char *argv[])
     };
     static struct option long_options[] = {
         {"statistics", no_argument, NULL, 's'},
+        {"more", no_argument, NULL, 'm'},
         {"timeout", required_argument, NULL, 't'},
         {"help", no_argument, NULL, 'h'},
         {"version", no_argument, NULL, 'V'},
@@ -95,6 +106,10 @@ parse_options(int argc, char *argv[])
             print_statistics = true;
             break;
 
+        case 'm':
+            verbosity++;
+            break;
+
         case 't':
             timeout = strtoul(optarg, NULL, 10);
             if (timeout <= 0) {
@@ -718,6 +733,201 @@ do_parse_actions(int argc, char *argv[])
     }
 }
 
+struct actions_for_flow {
+    struct hmap_node hmap_node;
+    struct flow flow;
+    struct ofpbuf actions;
+};
+
+static struct actions_for_flow *
+get_actions_for_flow(struct hmap *actions_per_flow, const struct flow *flow)
+{
+    uint32_t hash = flow_hash(flow, 0);
+    struct actions_for_flow *af;
+
+    HMAP_FOR_EACH_WITH_HASH (af, hmap_node, hash, actions_per_flow) {
+        if (flow_equal(&af->flow, flow)) {
+            return af;
+        }
+    }
+
+    af = xmalloc(sizeof *af);
+    af->flow = *flow;
+    ofpbuf_init(&af->actions, 0);
+    hmap_insert(actions_per_flow, &af->hmap_node, hash);
+    return af;
+}
+
+static int
+compare_actions_for_flow(const void *a_, const void *b_)
+{
+    struct actions_for_flow *const *a = a_;
+    struct actions_for_flow *const *b = b_;
+
+    return flow_compare_3way(&(*a)->flow, &(*b)->flow);
+}
+
+static int
+compare_output_actions(const void *a_, const void *b_)
+{
+    const struct nlattr *a = a_;
+    const struct nlattr *b = b_;
+    uint32_t a_port = nl_attr_get_u32(a);
+    uint32_t b_port = nl_attr_get_u32(b);
+
+    return a_port < b_port ? -1 : a_port > b_port;
+}
+
+static void
+sort_output_actions__(struct nlattr *first, struct nlattr *end)
+{
+    size_t bytes = (uint8_t *) end - (uint8_t *) first;
+    size_t n = bytes / NL_A_U32_SIZE;
+
+    assert(bytes % NL_A_U32_SIZE == 0);
+    qsort(first, n, NL_A_U32_SIZE, compare_output_actions);
+}
+
+static void
+sort_output_actions(struct nlattr *actions, size_t length)
+{
+    struct nlattr *first_output = NULL;
+    struct nlattr *a;
+    int left;
+
+    NL_ATTR_FOR_EACH (a, left, actions, length) {
+        if (nl_attr_type(a) == OVS_ACTION_ATTR_OUTPUT) {
+            if (!first_output) {
+                first_output = a;
+            }
+        } else {
+            if (first_output) {
+                sort_output_actions__(first_output, a);
+                first_output = NULL;
+            }
+        }
+    }
+    if (first_output) {
+        uint8_t *end = (uint8_t *) actions + length;
+        sort_output_actions__(first_output, (struct nlattr *) end);
+    }
+}
+
+/* usage: "ovs-dpctl normalize-actions FLOW ACTIONS" where FLOW and ACTIONS
+ * have the syntax used by "ovs-dpctl dump-flows".
+ *
+ * This command prints ACTIONS in a format that shows what happens for each
+ * VLAN, independent of the order of the ACTIONS.  For example, there is more
+ * than one way to output a packet on VLANs 9 and 11, but this command will
+ * print the same output for any form.
+ *
+ * The idea here generalizes beyond VLANs (e.g. to setting other fields) but
+ * so far the implementation only covers VLANs. */
+static void
+do_normalize_actions(int argc, char *argv[])
+{
+    struct shash port_names;
+    struct ofpbuf keybuf;
+    struct flow flow;
+    struct ofpbuf odp_actions;
+    struct hmap actions_per_flow;
+    struct actions_for_flow **afs;
+    struct actions_for_flow *af;
+    struct nlattr *a;
+    size_t n_afs;
+    struct ds s;
+    int left;
+    int i;
+
+    ds_init(&s);
+
+    shash_init(&port_names);
+    for (i = 3; i < argc; i++) {
+        char name[16];
+        int number;
+        int n = -1;
+
+        if (sscanf(argv[i], "%15[^=]=%d%n", name, &number, &n) > 0 && n > 0) {
+            shash_add(&port_names, name, (void *) number);
+        } else {
+            ovs_fatal(0, "%s: expected NAME=NUMBER", argv[i]);
+        }
+    }
+
+    /* Parse flow key. */
+    ofpbuf_init(&keybuf, 0);
+    run(odp_flow_key_from_string(argv[1], &port_names, &keybuf),
+        "odp_flow_key_from_string");
+
+    ds_clear(&s);
+    odp_flow_key_format(keybuf.data, keybuf.size, &s);
+    printf("input flow: %s\n", ds_cstr(&s));
+
+    run(odp_flow_key_to_flow(keybuf.data, keybuf.size, &flow),
+        "odp_flow_key_to_flow");
+    ofpbuf_uninit(&keybuf);
+
+    /* Parse actions. */
+    ofpbuf_init(&odp_actions, 0);
+    run(odp_actions_from_string(argv[2], &port_names, &odp_actions),
+        "odp_actions_from_string");
+
+    if (verbosity) {
+        ds_clear(&s);
+        format_odp_actions(&s, odp_actions.data, odp_actions.size);
+        printf("input actions: %s\n", ds_cstr(&s));
+    }
+
+    hmap_init(&actions_per_flow);
+    NL_ATTR_FOR_EACH (a, left, odp_actions.data, odp_actions.size) {
+        if (nl_attr_type(a) == OVS_ACTION_ATTR_POP_VLAN) {
+            flow.vlan_tci = htons(0);
+            continue;
+        }
+
+        if (nl_attr_type(a) == OVS_ACTION_ATTR_PUSH_VLAN) {
+            const struct ovs_action_push_vlan *push;
+
+            push = nl_attr_get_unspec(a, sizeof *push);
+            flow.vlan_tci = push->vlan_tci;
+            continue;
+        }
+
+        af = get_actions_for_flow(&actions_per_flow, &flow);
+        nl_msg_put_unspec(&af->actions, nl_attr_type(a),
+                          nl_attr_get(a), nl_attr_get_size(a));
+    }
+
+    n_afs = hmap_count(&actions_per_flow);
+    afs = xmalloc(n_afs * sizeof *afs);
+    i = 0;
+    HMAP_FOR_EACH (af, hmap_node, &actions_per_flow) {
+        afs[i++] = af;
+    }
+    assert(i == n_afs);
+
+    qsort(afs, n_afs, sizeof *afs, compare_actions_for_flow);
+
+    for (i = 0; i < n_afs; i++) {
+        const struct actions_for_flow *af = afs[i];
+
+        sort_output_actions(af->actions.data, af->actions.size);
+
+        if (af->flow.vlan_tci != htons(0)) {
+            printf("vlan(vid=%"PRIu16",pcp=%d): ",
+                   vlan_tci_to_vid(af->flow.vlan_tci),
+                   vlan_tci_to_pcp(af->flow.vlan_tci));
+        } else {
+            printf("no vlan: ");
+        }
+
+        ds_clear(&s);
+        format_odp_actions(&s, af->actions.data, af->actions.size);
+        puts(ds_cstr(&s));
+    }
+    ds_destroy(&s);
+}
+
 static const struct command all_commands[] = {
     { "add-dp", 1, INT_MAX, do_add_dp },
     { "del-dp", 1, 1, do_del_dp },
@@ -732,6 +942,7 @@ static const struct command all_commands[] = {
 
     /* Undocumented commands for testing. */
     { "parse-actions", 1, INT_MAX, do_parse_actions },
+    { "normalize-actions", 2, INT_MAX, do_normalize_actions },
 
     { NULL, 0, 0, NULL },
 };