[ovs-dev] [PATCH] ofp-parse: Fix parsing of "resubmit" action.

Ben Pfaff blp at nicira.com
Thu Jul 19 17:04:21 UTC 2012


On Thu, Jul 19, 2012 at 12:45:19AM -0700, Justin Pettit wrote:
> The new abstraction introduced in commit f25d0cf (Introduce ofpacts, an
> abstraction of OpenFlow actions.) provides a mechanism for storing the
> original action type for when converting from the internal representation
> to a public one may be ambiguous.  The "resubmit" action must
> distinguish between OFPUTIL_NXAST_RESUBMIT_TABLE and
> OFPUTIL_NXAST_RESUBMIT, so the parsing function needs to properly fill
> out the "compat" member of the ofpact structure.
> 
> Bug #8899
> Reported-by: Luca Giraudo <lgiraudo at nicira.com>
> Signed-off-by: Justin Pettit <jpettit at nicira.com>

Good catch.

Hmm.  I hadn't considered this issue when I wrote the ofpacts code.  If
I understand what's going on, it's essentially that ofpacts that differ
bitwise can yield the same representation as a string (or as OpenFlow
1.0 actions).  That's going to continue to come up, even if we paper
over it in this one case.

So, I'd suggest that, instead of comparing ofpacts bytewise, we compare
some other representation.  We could, for example, format the old rule
and the new rule as strings, then if they're the same just don't print
the difference (it's false).  This would avoid printing false
differences too.

Do you have a test case we could add to the unit tests?

Here's an implementation:

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Thu, 19 Jul 2012 10:03:33 -0700
Subject: [PATCH] ovs-ofctl: Avoid printing false differences on "ovs-ofctl diff-flows".

It is possible for "struct ofpact"s to differ bytewise even if they are
equivalent when converted to another representation, such as OpenFlow 1.0
action format or a string representation.  This can cause "ovs-ofctl
diff-flows" to print surprising false "differences", e.g. as in the bug
report:

    - actions=resubmit(,1)
    + actions=resubmit(,1)

This commit fixes the problem by comparing not just the ofpacts but also
the string representation and printing a difference only if both differ.

Bug #8899.
Reported-by: Luca Giraudo <lgiraudo at nicira.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 utilities/ovs-ofctl.c |   47 ++++++++++++++++++++++++++++-------------------
 1 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 39c3dae..afb2656 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1740,13 +1740,20 @@ fte_version_equals(const struct fte_version *a, const struct fte_version *b)
                              b->ofpacts, b->ofpacts_len));
 }
 
-/* Prints 'version' on stdout.  Expects the caller to have printed the rule
- * associated with the version. */
+/* Clears 's', then if 's' has a version 'index', formats 'fte' and version
+ * 'index' into 's', preceded by 'leader' and followed by a new-line. */
 static void
-fte_version_print(const struct fte_version *version)
+fte_version_format(const struct fte *fte, int index, char leader, struct ds *s)
 {
-    struct ds s;
+    const struct fte_version *version = fte->versions[index];
+
+    ds_clear(s);
+    if (!version) {
+        return;
+    }
 
+    ds_put_char(s, leader);
+    cls_rule_format(&fte->rule, s);
     if (version->cookie != htonll(0)) {
         printf(" cookie=0x%"PRIx64, ntohll(version->cookie));
     }
@@ -1757,10 +1764,10 @@ fte_version_print(const struct fte_version *version)
         printf(" hard_timeout=%"PRIu16, version->hard_timeout);
     }
 
-    ds_init(&s);
-    ofpacts_format(version->ofpacts, version->ofpacts_len, &s);
-    printf(" %s\n", ds_cstr(&s));
-    ds_destroy(&s);
+    ds_put_char(s, ' ');
+    ofpacts_format(version->ofpacts, version->ofpacts_len, s);
+
+    ds_put_char(s, '\n');
 }
 
 static struct fte *
@@ -2067,33 +2074,35 @@ ofctl_diff_flows(int argc OVS_UNUSED, char *argv[])
     bool differences = false;
     struct cls_cursor cursor;
     struct classifier cls;
+    struct ds a_s, b_s;
     struct fte *fte;
 
     classifier_init(&cls);
     read_flows_from_source(argv[1], &cls, 0);
     read_flows_from_source(argv[2], &cls, 1);
 
+    ds_init(&a_s);
+    ds_init(&b_s);
+
     cls_cursor_init(&cursor, &cls, NULL);
     CLS_CURSOR_FOR_EACH (fte, rule, &cursor) {
         struct fte_version *a = fte->versions[0];
         struct fte_version *b = fte->versions[1];
 
         if (!a || !b || !fte_version_equals(a, b)) {
-            char *rule_s = cls_rule_to_string(&fte->rule);
-            if (a) {
-                printf("-%s", rule_s);
-                fte_version_print(a);
+            fte_version_format(fte, 0, '-', &a_s);
+            fte_version_format(fte, 1, '+', &b_s);
+            if (strcmp(ds_cstr(&a_s), ds_cstr(&b_s))) {
+                fputs(ds_cstr(&a_s), stdout);
+                fputs(ds_cstr(&b_s), stdout);
+                differences = true;
             }
-            if (b) {
-                printf("+%s", rule_s);
-                fte_version_print(b);
-            }
-            free(rule_s);
-
-            differences = true;
         }
     }
 
+    ds_destroy(&a_s);
+    ds_destroy(&b_s);
+
     fte_free_all(&cls);
 
     if (differences) {
-- 
1.7.2.5




More information about the dev mailing list