From: Ben Pfaff <blp@nicira.com>
Date: Mon, 23 Jul 2012 17:16:31 +0000 (-0700)
Subject: ovs-ofctl: Avoid printing false differences on "ovs-ofctl diff-flows".
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?p=openvswitch;a=commitdiff_plain;h=98f7f427bf8bb339d4d1f1df1ff9b3310f8f5bc4

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@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
---

diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 8e3c5f77..b1b4c6c1 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -1962,3 +1962,28 @@ AT_CHECK([ovs-ofctl diff-flows add-flows.txt br0 | sort], [0], [expout])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+dnl ofpacts that differ bytewise don't necessarily differ when
+dnl converted to another representation, such as OpenFlow 1.0
+dnl or to a string.  "resubmit(,1)" is an example of an action
+dnl of this type: "ofpact_resubmit"s can differ in their "compat"
+dnl values even though this doesn't affect the string format.
+dnl
+dnl This test checks that "ovs-ofctl diff-flows" doesn't report
+dnl false ofpacts differences.
+AT_SETUP([ovs-ofctl diff-flows - suppress false differences])
+OVS_VSWITCHD_START
+AT_DATA([flows.txt], [actions=resubmit(,1)
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-ofctl diff-flows br0 flows.txt])
+AT_CHECK([ovs-ofctl add-flow br0 idle_timeout=60,dl_vlan=9,actions=output:1])
+AT_CHECK([ovs-ofctl diff-flows br0 flows.txt], [2], [dnl
+-dl_vlan=9 idle_timeout=60 actions=output:1
+])
+AT_CHECK([ovs-ofctl add-flow br0 hard_timeout=120,cookie=1234,dl_vlan=9,actions=output:1])
+AT_CHECK([ovs-ofctl diff-flows flows.txt br0], [2], [dnl
++dl_vlan=9 cookie=0x4d2 hard_timeout=120 actions=output:1
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 4a5a6013..3f64178c 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1740,27 +1740,33 @@ 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', followed by a new-line. */
 static void
-fte_version_print(const struct fte_version *version)
+fte_version_format(const struct fte *fte, int index, struct ds *s)
 {
-    struct ds s;
+    const struct fte_version *version = fte->versions[index];
 
+    ds_clear(s);
+    if (!version) {
+        return;
+    }
+
+    cls_rule_format(&fte->rule, s);
     if (version->cookie != htonll(0)) {
-        printf(" cookie=0x%"PRIx64, ntohll(version->cookie));
+        ds_put_format(s, " cookie=0x%"PRIx64, ntohll(version->cookie));
     }
     if (version->idle_timeout != OFP_FLOW_PERMANENT) {
-        printf(" idle_timeout=%"PRIu16, version->idle_timeout);
+        ds_put_format(s, " idle_timeout=%"PRIu16, version->idle_timeout);
     }
     if (version->hard_timeout != OFP_FLOW_PERMANENT) {
-        printf(" hard_timeout=%"PRIu16, version->hard_timeout);
+        ds_put_format(s, " 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 +2073,39 @@ 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);
-            }
-            if (b) {
-                printf("+%s", rule_s);
-                fte_version_print(b);
+            fte_version_format(fte, 0, &a_s);
+            fte_version_format(fte, 1, &b_s);
+            if (strcmp(ds_cstr(&a_s), ds_cstr(&b_s))) {
+                if (a_s.length) {
+                    printf("-%s", ds_cstr(&a_s));
+                }
+                if (b_s.length) {
+                    printf("+%s", ds_cstr(&b_s));
+                }
+                differences = true;
             }
-            free(rule_s);
-
-            differences = true;
         }
     }
 
+    ds_destroy(&a_s);
+    ds_destroy(&b_s);
+
     fte_free_all(&cls);
 
     if (differences) {