[ovs-dev] [tests+nxm-ofctl 41/42] ovs-ofctl: Add NXM support.

Ben Pfaff blp at nicira.com
Tue Dec 7 21:31:52 UTC 2010


On Tue, Dec 07, 2010 at 12:42:54AM -0800, Justin Pettit wrote:
> On Nov 23, 2010, at 2:44 PM, Ben Pfaff wrote:
> 
> > +parse_ofp_str(struct flow_mod *fm, uint8_t *table_idx,
> > +              struct ofpbuf *actions, char *string)
> > {
> > ...
> > +    fm->cookie = htonl(0);
> 
> Not that the effect will be different, but I believe that should be a
> htonll(0).

Thanks, fixed.

> > @@ -629,40 +640,41 @@ parse_ofp_str(struct parsed_flow *pf, struct ofpbuf *actions, char *string)
> > 
> > /* Parses 'string' as an OFPT_FLOW_MOD with command 'command' (one of OFPFC_*)
> >  * and returns an ofpbuf that contains it. */
> > +void
> > +parse_ofp_flow_mod_str(struct list *packets, enum nx_flow_format *cur_format,
> > +                       char *string, uint16_t command)
> > {
> 
> This function is no longer strictly limited to OFPT_FLOW_MOD, right?

Oh, and the comment needs other updates too.  Thanks, I fixed this.

> > /* Parses an OFPT_FLOW_MOD with subtype OFPFC_ADD from 'stream' and returns an
> > + * ofpbuf that contains it.  Returns false if end-of-file is reached before
> > + * reading a flow, otherwise true. */
> > +bool
> > +parse_ofp_add_flow_file(struct list *packets, enum nx_flow_format *cur,
> > +                        FILE *stream)
> > {
> 
> Same here.

Ditto.

> > +/* Returns an OpenFlow message that can be used to set the flow format to
> > + * 'flow_format'.  */
> > +struct ofpbuf *
> > +ofputil_make_set_flow_format(enum nx_flow_format flow_format)
> > +{
> > ...
> > +        sff = make_nxmsg(sizeof *sff, NXT_SET_FLOW_FORMAT, &msg);
> > +        sff->format = flow_format;
> 
> Shouldn't that have a htonl() on it?

Yes.  (It seems that I found this in my own testing a while back; it was
already fixed in my copy here.)

> > --- a/utilities/ovs-ofctl.8.in
> > +++ b/utilities/ovs-ofctl.8.in
> > @@ -620,6 +620,37 @@ described in \fBFlow Syntax\fR, above.
> > ...
> > +.IP "\fBtun_id_from_cookie\fR"
> > +This Nicira extension to OpenFlow adds minimal and limited support for
> > +\fBtun_id\fR, but it does not support any other Nicira flow
> > +extensions.
> 
> I hope we can retire this mode before too long.  What do you think of
> specifically discouraging people from using it?

I added "(This flow format is deprecated.)" to the description.

> > +static bool
> > +try_set_flow_format(struct vconn *vconn, enum nx_flow_format flow_format)
> > +{
> > +    struct ofpbuf *sff, *reply;
> > +
> > +    sff = ofputil_make_set_flow_format(NXFF_NXM);
> 
> Shouldn't that be "flow_format" instead of "NXFF_NXM"?

Oops, yes.

> > static void
> > +do_dump_flows__(int argc, char *argv[], bool aggregate)
> > {
> > ...
> > +    open_vconn(argv[1], &vconn);
> > +    flow_format = negotiate_highest_flow_format(vconn, &fsr.match);
> > +    request = ofputil_encode_flow_stats_request(&fsr, flow_format);
> >     dump_stats_transaction(argv[1], request);
> > }
> 
> Should you be closing that vconn you opened?

Thanks, fixed.




More information about the dev mailing list