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

[Justin Pettit]

Vendor error messages seem like a good candidate for inclusion in OpenFlow 1.1.  Perhaps Martin should bring it up at the next meeting...

> + * 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".

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

> +/* Error codes.
>  *
> + * 31                                                     0
> + *  +-----------------------------------------------------+
> + *  |                          0                          |  success
> + *  +-----------------------------------------------------+
>  *
> + * 31 30                                                  0
> + *  +-+---------------------------------------------------+
> + *  |0|                    errno value                    |  errno value
> + *  +-+---------------------------------------------------+
>  *
> + * 31 30     26               16                          0
> + *  +-+-------+----------------+--------------------------+
> + *  |1|   0   |      type      |           code           |  standard OpenFlow
> + *  +-+-------+----------------+--------------------------+  error
>  *
> + * 31 30     26               16                          0
> + *  +-+-------+----------------+--------------------------+  Nicira
> + *  |1| vendor|      type      |           code           |  NXET_VENDOR
> + *  +-+-------+----------------+--------------------------+  error extension

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

> + * The 'vendor' field holds one of the OFPUTIL_VENDOR_* codes defined below.

Wasn't it defined "above"?

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

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

--Justin