[ovs-dev] [tests+nxm-ofctl 40/42] ofp-print: Implement printing for OFPUTIL_NXT_FLOW_MOD.

Ben Pfaff blp at nicira.com
Tue Dec 7 21:22:33 UTC 2010


On Mon, Dec 06, 2010 at 06:50:41PM -0800, Justin Pettit wrote:
> On Nov 23, 2010, at 2:44 PM, Ben Pfaff wrote:
> 
> > +    error = ofputil_decode_flow_mod(&fm, oh, NXFF_OPENFLOW10);
> 
> In the function ofputil_decode_flow_mod(), I think it would be useful
> to mention in the comment that the third element is only used when the
> message is not OFPUTIL_NXT_FLOW_MOD.  It's a little confusing
> otherwise.

Thanks, I updated the comment to:

/* Converts an OFPT_FLOW_MOD or NXT_FLOW_MOD message 'oh' into an abstract
 * flow_mod in 'fm'.  Returns 0 if successful, otherwise an OpenFlow error
 * code.
 *
 * For OFPT_FLOW_MOD messages, 'flow_format' should be the current flow format
 * at the time when the message was received.  Otherwise 'flow_format' is
 * ignored.
 *
 * Does not validate the flow_mod actions. */

> >     default:
> > -        ds_put_format(string, "cmd:%d", ntohs(ofm->command));
> > +        ds_put_format(s, "cmd%d", fm.command);
> 
> Did you mean to remove the colon?

I don't remember.  For now I put it back.

> > +    if (fm.cr.priority != 32768 && verbosity >= 3) {
> 
> I think it would be clearer to use OFP_DEFAULT_PRIORITY instead of 32768.

Done, thanks.

> > static void
> > +ofp_print_error(struct ds *string, int error)
> > +{
> > ...
> > +    ds_put_format(string, " ***decode error type%d(%s) code%d(%s)***",
> > +                  type, lookup_error_type(type),
> > +                  code, lookup_error_code(type, code));
> 
> I'm noticing a trend that you are likely dropping the colons on purpose...

I put them back.

> > static void
> > +ofp_print_nxt_tun_id_from_cookie(struct ds *string,
> > +                                 const struct nxt_tun_id_cookie *ntic)
> > +{
> > +    ds_put_format(string, " set=%"PRIu32, ntic->set);
> > +}
> 
> The element "ntic->set" is only 8 bits, but you seem to be printing it
> as if it were 32 bits.

Oops.  Thanks, fixed.




More information about the dev mailing list