Looks good. The do_normalize_actions() function is inconsistent about what
memory it cleans up before exiting. Since it's short-lived, it doesn't really
matter, of course. I also think the function could use some explanation about
what it's doing--it's pretty hard to see by looking at the code alone. The
Perl version supplied a bit of a description, but I think even that could have
used a couple more sentences.
--Justin
On Oct 26, 2011, at 10:09 AM, Ben Pfaff wrote:
> 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.
> ---
> tests/automake.mk | 1 -
> tests/compare-odp-actions.pl | 66 --------------
> tests/ofproto-dpif.at | 6 +-
> utilities/ovs-dpctl.c | 206 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 210 insertions(+), 69 deletions(-)
> delete mode 100644 tests/compare-odp-actions.pl
>
> diff --git a/tests/automake.mk b/tests/automake.mk
> index f11e291..09de521 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 b18a593..0000000
> --- 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 ec5c238..312ec1f 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 69a66b6..30e06b4 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,196 @@ 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);
> + }
> +}
> +
> +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
> + && nl_attr_get_u16(a) == OVS_KEY_ATTR_8021Q) {
> + flow.vlan_tci = htons(0);
> + continue;
> + }
> +
> + if (nl_attr_type(a) == OVS_ACTION_ATTR_PUSH) {
> + const struct nlattr *nested = nl_attr_get(a);
> +
> + if (nl_attr_type(nested) == OVS_KEY_ATTR_8021Q) {
> + const struct ovs_key_8021q *q_key;
> +
> + q_key = nl_attr_get_unspec(nested, sizeof(*q_key));
> + flow.vlan_tci = q_key->q_tci | htons(VLAN_CFI);
> + 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 +937,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 },
> };
> --
> 1.7.2.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
