This bug fix still needs a review. Also, Kevin, if you can verify that
it fixes the behavior you see, that would also be helpful.
On Thu, Jul 06, 2017 at 04:40:30PM -0700, Ben Pfaff wrote:
> The ovs-ofctl "diff-flows" and "replace-flows" command compare the flows
> in two flow tables. Until now, the "replace-flows" command has considered
> certain almost meaningless differences related to the version of OpenFlow
> used to add a flow as significant, which caused it to replace a flow by an
> identical-in-practice version, e.g. in the following, the "replace-flows"
> command prints a FLOW_MOD that adds the flow that was already added
> previously:
>
> $ cat > flows
> actions=resubmit(,1)
> $ ovs-vsctl add-br br0
> $ ovs-ofctl del-flows br0
> $ ovs-ofctl add-flows br0 flows
> $ ovs-ofctl -vvconn replace-flows br0 flows 2>&1 | grep FLOW_MOD
>
> Re-adding an existing flow has some effects, for example, it resets the
> flow's duration, so it's better to avoid it.
>
> This commit fixes the problem using the same trick previously used for a
> similar problem with the "diff-flows" command, which was fixed in commit
> 98f7f427bf8b ("ovs-ofctl: Avoid printing false differences on "ovs-ofctl
> diff-flows".").
>
> Reported-by: Kevin Lin <[email protected]>
> Signed-off-by: Ben Pfaff <[email protected]>
> ---
> include/openvswitch/ofp-actions.h | 2 ++
> lib/ofp-actions.c | 24 ++++++++++++++++++++++++
> utilities/ovs-ofctl.c | 18 ++++++++----------
> 3 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/include/openvswitch/ofp-actions.h
> b/include/openvswitch/ofp-actions.h
> index 7b4aa9201d9b..0dcb6452ea02 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1013,6 +1013,8 @@ bool ofpacts_output_to_group(const struct ofpact[],
> size_t ofpacts_len,
> uint32_t group_id);
> bool ofpacts_equal(const struct ofpact a[], size_t a_len,
> const struct ofpact b[], size_t b_len);
> +bool ofpacts_equal_stringwise(const struct ofpact a[], size_t a_len,
> + const struct ofpact b[], size_t b_len);
> const struct mf_field *ofpact_get_mf_dst(const struct ofpact *ofpact);
> uint32_t ofpacts_get_meter(const struct ofpact[], size_t ofpacts_len);
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index ae27d9d88080..868b511dc908 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -8316,6 +8316,8 @@ ofpacts_output_to_group(const struct ofpact *ofpacts,
> size_t ofpacts_len,
> return false;
> }
>
> +/* Returns true if the 'a_len' bytes of actions in 'a' and the 'b_len' bytes
> of
> + * actions in 'b' are bytewise identical. */
> bool
> ofpacts_equal(const struct ofpact *a, size_t a_len,
> const struct ofpact *b, size_t b_len)
> @@ -8323,6 +8325,28 @@ ofpacts_equal(const struct ofpact *a, size_t a_len,
> return a_len == b_len && !memcmp(a, b, a_len);
> }
>
> +/* Returns true if the 'a_len' bytes of actions in 'a' and the 'b_len' bytes
> of
> + * actions in 'b' are identical when formatted as strings. (Converting
> actions
> + * to string form suppresses some rarely meaningful differences, such as the
> + * 'compat' member of actions.) */
> +bool
> +ofpacts_equal_stringwise(const struct ofpact *a, size_t a_len,
> + const struct ofpact *b, size_t b_len)
> +{
> + struct ds a_s = DS_EMPTY_INITIALIZER;
> + struct ds b_s = DS_EMPTY_INITIALIZER;
> +
> + ofpacts_format(a, a_len, NULL, &a_s);
> + ofpacts_format(b, b_len, NULL, &b_s);
> +
> + bool equal = !strcmp(ds_cstr(&a_s), ds_cstr(&b_s));
> +
> + ds_destroy(&a_s);
> + ds_destroy(&b_s);
> +
> + return equal;
> +}
> +
> /* Finds the OFPACT_METER action, if any, in the 'ofpacts_len' bytes of
> * 'ofpacts'. If found, returns its meter ID; if not, returns 0.
> *
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 6fb2cc08de50..5b7f1b02104f 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -3146,8 +3146,8 @@ fte_version_equals(const struct fte_version *a, const
> struct fte_version *b)
> && a->hard_timeout == b->hard_timeout
> && a->importance == b->importance
> && a->table_id == b->table_id
> - && ofpacts_equal(a->ofpacts, a->ofpacts_len,
> - b->ofpacts, b->ofpacts_len));
> + && ofpacts_equal_stringwise(a->ofpacts, a->ofpacts_len,
> + b->ofpacts, b->ofpacts_len));
> }
>
> /* Clears 's', then if 's' has a version 'index', formats 'fte' and version
> @@ -3656,15 +3656,13 @@ ofctl_diff_flows(struct ovs_cmdl_context *ctx)
> if (!a || !b || !fte_version_equals(a, b)) {
> fte_version_format(&fte_state, fte, 0, &a_s);
> fte_version_format(&fte_state, 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;
> + if (a_s.length) {
> + printf("-%s", ds_cstr(&a_s));
> + }
> + if (b_s.length) {
> + printf("+%s", ds_cstr(&b_s));
> }
> + differences = true;
> }
> }
> }
> --
> 2.10.2
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
