[ovs-dev] [ovs-ofctl 03/10] ovs-ofctl: Check min flow format support in negotiate_highest_flow_format().

[Ethan Jackson]

Looks good to me.  I didn't closely review the unit test updates.

Ethan

On Fri, Mar 11, 2011 at 1:20 PM, Ben Pfaff <blp at nicira.com> wrote:
> When the -F option wasn't set, or if it was set to an invalid flow format
> for the match, this code would happily select a flow format that did not
> select the user's requested match if the switch didn't support an
> advanced-enough flow format.  This fixes the problem.  It also changes
> behavior in the case where the user specifies a flow format that cannot
> represent the match, changing this from a warning to a fatal error; this
> is consistent with -F behavior for flow_mod commands.
> ---
>  tests/ovs-ofctl.at    |   22 ++++++++++++++++++++++
>  utilities/ovs-ofctl.c |   46 ++++++++++++++++++++++++++--------------------
>  2 files changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index dfdebd3..466ade6 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -486,3 +486,25 @@ NXST_FLOW reply:
>  ])
>  OFPROTO_STOP
>  AT_CLEANUP
> +
> +dnl Check that "-F openflow10" is really honored on dump-flows.
> +dnl (If it isn't, then dump-flows will show the register match.)
> +AT_SETUP([ovs-ofctl dump-flows honors -F option])
> +OFPROTO_START
> +AT_CHECK([ovs-ofctl add-flow br0 reg0=0x12345,actions=drop])
> +AT_CHECK([ovs-ofctl -F openflow10 dump-flows br0 | STRIP_XIDS | STRIP_DURATION], [0], [dnl
> +OFPST_FLOW reply:
> + cookie=0x0, duration=?s, table_id=0, n_packets=0, n_bytes=0,  actions=drop
> +])
> +OFPROTO_STOP
> +AT_CLEANUP
> +
> +dnl Check that "-F openflow10" fails on dump-flows if the requested match
> +dnl can't be represented in OpenFlow 1.0.
> +AT_SETUP([ovs-ofctl dump-flows rejects bad -F option])
> +OFPROTO_START
> +AT_CHECK([ovs-ofctl -F openflow10 dump-flows unix:br0.mgmt reg0=0xabcdef], [1], [],
> +  [ovs-ofctl: unix:br0.mgmt: cannot use requested flow format nxm for specified flow
> +])
> +OFPROTO_STOP
> +AT_CLEANUP
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index f7605f7..5c89c76 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -540,33 +540,39 @@ static enum nx_flow_format
>  negotiate_highest_flow_format(struct vconn *vconn, const struct cls_rule *rule,
>                               bool cookie_support, ovs_be64 cookie)
>  {
> -    int flow_format;
> +    enum nx_flow_format min_format;
>
> +    min_format = ofputil_min_flow_format(rule, cookie_support, cookie);
>     if (preferred_flow_format != -1) {
> -        enum nx_flow_format min_format;
> +        if (preferred_flow_format < min_format) {
> +            ovs_fatal(0, "%s: cannot use requested flow format %s for "
> +                      "specified flow", vconn_get_name(vconn),
> +                      ofputil_flow_format_to_string(min_format));
> +        }
>
> -        min_format = ofputil_min_flow_format(rule, cookie_support, cookie);
> -        if (preferred_flow_format >= min_format) {
> -            set_flow_format(vconn, preferred_flow_format);
> -            return preferred_flow_format;
> +        set_flow_format(vconn, preferred_flow_format);
> +        return preferred_flow_format;
> +    } else {
> +        enum nx_flow_format flow_format;
> +
> +        if (try_set_flow_format(vconn, NXFF_NXM)) {
> +            flow_format = NXFF_NXM;
> +        } else if (try_set_flow_format(vconn, NXFF_TUN_ID_FROM_COOKIE)) {
> +            flow_format = NXFF_TUN_ID_FROM_COOKIE;
> +        } else {
> +            flow_format = NXFF_OPENFLOW10;
>         }
>
> -        VLOG_WARN("%s: cannot use requested flow format %s for "
> -                  "specified flow", vconn_get_name(vconn),
> -                  ofputil_flow_format_to_string(min_format));
> -    }
> +        if (flow_format < min_format) {
> +            ovs_fatal(0, "%s: cannot use switch's most advanced flow format "
> +                      "%s for specified flow", vconn_get_name(vconn),
> +                      ofputil_flow_format_to_string(min_format));
> +        }
>
> -    if (try_set_flow_format(vconn, NXFF_NXM)) {
> -        flow_format = NXFF_NXM;
> -    } else if (try_set_flow_format(vconn, NXFF_TUN_ID_FROM_COOKIE)) {
> -        flow_format = NXFF_TUN_ID_FROM_COOKIE;
> -    } else {
> -        flow_format = NXFF_OPENFLOW10;
> +        VLOG_DBG("%s: negotiated flow format %s", vconn_get_name(vconn),
> +                 ofputil_flow_format_to_string(flow_format));
> +        return flow_format;
>     }
> -
> -    VLOG_DBG("%s: negotiated flow format %s", vconn_get_name(vconn),
> -             ofputil_flow_format_to_string(flow_format));
> -    return flow_format;
>  }
>
>  static void
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>