On Thu, Jul 19, 2012 at 10:28:03AM -0700, Justin Pettit wrote:
>
> On Jul 19, 2012, at 10:04 AM, Ben Pfaff wrote:
>
> > 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 <lgira...@nicira.com>
> >> Signed-off-by: Justin Pettit <jpet...@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.
>
> Yes, I think this is a better approach and more future-proof as new actions
> get added.
>
> > Do you have a test case we could add to the unit tests?
>
> No, I don't have one written up beyond what Luca described in the bug report.
OK, I wrote one. I'm glad I did since the patch I posted didn't
work. Here's a fixed version for final review.
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <b...@nicira.com>
Date: Thu, 19 Jul 2012 11:18:25 -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 <lgira...@nicira.com>
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
tests/ovs-ofctl.at | 17 ++++++++++++++++
utilities/ovs-ofctl.c | 50 ++++++++++++++++++++++++++++++------------------
2 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index d6ade55..213007f 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -1825,3 +1825,20 @@ 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])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 39c3dae..d4023fa 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1740,13 +1740,19 @@ 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));
}
@@ -1757,10 +1763,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 +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) {
--
1.7.2.5
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
