[ovs-dev] [wdp error reporting 4/5] ofp-util: Add infrastructure for vendor extensions to OpenFlow error codes.

Ben Pfaff blp at nicira.com
Fri Aug 20 17:26:58 UTC 2010


On Mon, Aug 16, 2010 at 04:16:54PM -0700, Justin Pettit wrote:
> > + * This extension attempts to address the problem by adding a generic "error
> > + * vendor extension".  The extension works as follows: use NX_OFPET_VENDOR as
> 
> I presume you mean "NXET_VENDOR" instead of "NX_OFPET_VENDOR".

Yes, thanks, fixed.

> > +static uint32_t
> > +vendor_code_to_id(uint8_t code)
> > +{
> > +    switch (code) {
> > +#define OFPUTIL_VENDOR(NAME, VENDOR_ID) case NAME: return VENDOR_ID;
> > +    default:
> > +        return 0;
> 
> Are you concerned that an invalid vendor will map to OpenFlow?  I know
> you handle it in the code below, but a different value might make the
> casual user of this function a little less confused.

It sounds worthwhile to avoid confusion, so I changed the error return
value to UINT32_MAX.

> I found these ASCII graphics a bit confusing.  What about something
> like the following?
> 
>     3                   2                   1
>     0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |1| vendor|       type        |              code             |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

All the digits make the diagram look shimmery.  I don't think Edward
Tufte would approve.

I changed it to:

 * We embed system errno values and OpenFlow standard and vendor extension
 * error codes into a single 31-bit space using the following encoding.
 * (Bit 31 is unused and assumed 0 to avoid negative "int" values.)
 *
 *   31                                                   0
 *  +------------------------------------------------------+
 *  |                           0                          |  success
 *  +------------------------------------------------------+
 *
 *   30 29                                                0
 *  +--+---------------------------------------------------+
 *  |0 |                    errno value                    |  errno value
 *  +--+---------------------------------------------------+
 *
 *   30 29   26 25            16 15                       0
 *  +--+-------+----------------+--------------------------+
 *  |1 |   0   |      type      |           code           |  standard OpenFlow
 *  +--+-------+----------------+--------------------------+  error
 *
 *   30 29   26 25            16 15                       0
 *  +--+-------+----------------+--------------------------+  Nicira
 *  | 1| vendor|      type      |           code           |  NXET_VENDOR
 *  +--+-------+----------------+--------------------------+  error extension

> > + * The 'vendor' field holds one of the OFPUTIL_VENDOR_* codes defined below.
> 
> Wasn't it defined "above"?

Yes, thanks.

> > +/* Returns the standard OpenFlow error with the specified 'type' and 'code' as
> > + * an integer. */
> > static inline int
> > ofp_mkerr(uint16_t type, uint16_t code)
> > {
> > -    assert(type > 0 && type <= 0x7fff);
> > -    return (type << 16) | code;
> > +    return (1 << 30) | (type << 16) | code;
> 
> Do you not want to enforce that the type is <= 0x3fff?

I guess that problem doesn't seem likely to me.

> > +/* Returns the OpenFlow vendor error with the specified 'vendor', 'type', and
> > + * 'code' as an integer.  'vendor' must be an OFPUTIL_VENDOR_* constant. */
> > +static inline int
> > +ofp_make_vendor_error(uint8_t vendor, uint16_t type, uint16_t code)
> > +{
> > +    return (1 << 30) | (vendor << 26) | (type << 16) | code;
> 
> Did you want to check that both vendor and type are valid here?

Might as well check the vendor, since that one is the most likely to be
wrong.  Thanks, I added that.




More information about the dev mailing list