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

Justin Pettit jpettit at nicira.com
Tue Dec 7 08:42:54 UTC 2010


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).

> @@ -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?

> /* 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.

> +/* 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?

> --- 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?

> +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"?

> 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!

--Justin






More information about the dev mailing list