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.
Do you have a test case we could add to the unit tests?
Here's an implementation:
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <b...@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 <lgira...@nicira.com>
Signed-off-by: Ben Pfaff <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
