From 98f7f427bf8bb339d4d1f1df1ff9b3310f8f5bc4 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 23 Jul 2012 10:16:31 -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 Signed-off-by: Ben Pfaff --- tests/ovs-ofctl.at | 25 +++++++++++++++++++ utilities/ovs-ofctl.c | 56 ++++++++++++++++++++++++++----------------- 2 files changed, 59 insertions(+), 22 deletions(-) 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) { -- 2.30.2