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 <[email protected]> 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 > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
