I added this comment:
/* 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. */
On Tue, Nov 15, 2011 at 11:04:32PM -0800, Justin Pettit wrote:
> 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
