[ovs-dev] [nxm 20/42] ofp-util: Add infrastructure for vendor extensions to OpenFlow error codes.

Justin Pettit jpettit at nicira.com
Thu Nov 4 02:27:08 UTC 2010


On Oct 28, 2010, at 10:27 AM, Ben Pfaff wrote:

> + * It would be better to have type-specific vendor extension, e.g. so that

"to have" -> "to have a"?

> + * OFPET_BAD_ACTION could be used with vendor-specific code values.  But
> + * OFPET_BAD_ACTION and most other standardized types already specify that
> + * their 'data' values are (the start of) the OpenFlow message being replied
> + * to, so there is no room to insert a vendor ID.
> + *
> + * Currently this extension is only implemented by Open vSwitch, but it seems
> + * like a reasonable candidate for future standardization.

It may be worth sending out a proposal to openflow-spec for possible OpenFlow 1.1 inclusion.

> +struct nx_vendor_error {
> +    ovs_be32 vendor;            /* Vendor ID as in struct ofp_vendor_header. */
> +    ovs_be16 type;              /* Vendor-defined type. */
> +    ovs_be16 code;              /* Vendor-defined subtype. */
> +    /* Followed by at least the first 64 bytes of the failed request. */
> +};

In the forthcoming extensible match patch, we use a 2-byte vendor identifier.  Do you think it makes sense to just use one vendor identifier format in our extensions?  My opinion is that going forward, it would be great if we could move to the 2-byte format in OpenFlow.

> +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 UINT32_MAX;
> +    }
> +}

I must be slow tonight, but how do these case statements get populated?  I'd think you'd need to reference OFPUTIL_VENDORS...

> +struct ofpbuf *
> +make_ofp_error_msg(int error, const struct ofp_header *oh)
> +{
> ...
> +        struct ofp_error_msg *oem;
> +        struct nx_vendor_error *ove;

Using our usual naming convention, I believe that should be "nve".

> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> 
> +/* OpenFlow vendors.
>  *
> + * These functions map vendor */

I feel like this sentence is incomplete.

> +/* Vendor error numbers currently used in Open vSwitch. */
> +#define OFPUTIL_VENDORS                                     \
> +    /*             vendor name              vendor value */ \
> +    OFPUTIL_VENDOR(OFPUTIL_VENDOR_OPENFLOW, 0x00000000)     \
> +    OFPUTIL_VENDOR(OFPUTIL_VENDOR_NICIRA,   0x00002320)

Is there a reason not to use the NX_VENDOR_ID definition?

> + *   31                                                   0
> + *  +------------------------------------------------------+
> + *  |                           0                          |  success
> + *  +------------------------------------------------------+
>  *
> + *   30 29                                                0
> + *  +--+---------------------------------------------------+
> + *  |0 |                    errno value                    |  errno value
> + *  +--+---------------------------------------------------+

It's a bit confusing (no pun intended) that "success" goes to 31, when the others go to 30.

> +/* 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;
> +}

It seem like it may be useful to warn (or assert) if the "type" is going to spill into the "vendor" portion.

> +/* 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_mkerr_vendor(uint8_t vendor, uint16_t type, uint16_t code)
> +{
> +    assert(vendor < OFPUTIL_N_VENDORS);
> +    return (1 << 30) | (vendor << 26) | (type << 16) | code;
> }

Same comment here.

--Justin






More information about the dev mailing list