[ovs-dev] [nxm 20/42] ofp-util: Add infrastructure for vendor extensions to OpenFlow error codes.
Ben Pfaff
blp at nicira.com
Thu Nov 4 17:36:45 UTC 2010
Thanks for the comments.
Did you realize that this is the second time that you have reviewed this
patch? The first time, on Aug. 16, was for the wdp branch. But your
comments are mostly different this time.
To avoid committing subtly different commit under the same title on
different branches, I'm going to apply your new comments as a second
commit just after the first.
On Wed, Nov 03, 2010 at 07:27:08PM -0700, Justin Pettit wrote:
> 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"?
Thanks, fixed.
> > + * 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.
Whatever I propose gets ignored. It would just waste my time.
> > +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.
This extension is already in use on the "wdp" branch. I don't want to
change it.
> > +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...
Oops, good catch. I changed it to this:
static uint32_t
vendor_code_to_id(uint8_t code)
{
switch (code) {
#define OFPUTIL_VENDOR(NAME, VENDOR_ID) case NAME: return VENDOR_ID;
OFPUTIL_VENDORS
#undef OFPUTIL_VENDOR
default:
return UINT32_MAX;
}
}
> > +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".
Sure, no problem, changed.
> > --- a/lib/ofp-util.h
> > +++ b/lib/ofp-util.h
> >
> > +/* OpenFlow vendors.
> > *
> > + * These functions map vendor */
>
> I feel like this sentence is incomplete.
Why would you think
I always write English like
> > +/* 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?
No, we might as well. I forgot we had it. Thanks.
> > + * 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.
That's just a typo, thanks, so I changed the labeling from 31 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.
You said that in the last review too:
> Justin Pettit writes:
> > 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_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.
Here's our exchange from last time:
> Justin Pettit writes:
> > 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.
Here's the series of commits I stacked on top of the original commit due
to your comments.
Thanks!
--8<--------------------------cut here-------------------------->8--
>From 4815ce5883602ab461b794dbc6de05998fc6d3cc Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Thu, 4 Nov 2010 10:32:13 -0700
Subject: [PATCH 1/4] Fix typos in comments.
---
include/openflow/nicira-ext.h | 2 +-
lib/ofp-util.h | 13 +++++++++----
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 55b3548..fcdbba4 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -42,7 +42,7 @@
* vendor-specific details, followed by at least 64 bytes of the failed
* request.
*
- * It would be better to have type-specific vendor extension, e.g. so that
+ * It would be better to have a type-specific vendor extension, e.g. so that
* 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
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index d72a50b..1c683ec 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -85,7 +85,12 @@ char *ofp_match_to_literal_string(const struct ofp_match *match);
/* OpenFlow vendors.
*
- * These functions map vendor */
+ * These functions map OpenFlow 32-bit vendor IDs (as used in struct
+ * ofp_vendor_header) into 4-bit values to embed in an "int". The 4-bit values
+ * are only used internally in Open vSwitch and never appear on the wire, so
+ * particular codes used are not important.
+ */
+
/* Vendor error numbers currently used in Open vSwitch. */
#define OFPUTIL_VENDORS \
/* vendor name vendor value */ \
@@ -106,19 +111,19 @@ enum ofputil_vendor_codes {
* 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
+ * 30 0
* +------------------------------------------------------+
* | 0 | success
* +------------------------------------------------------+
*
* 30 29 0
* +--+---------------------------------------------------+
- * |0 | errno value | errno value
+ * | 0| errno value | errno value
* +--+---------------------------------------------------+
*
* 30 29 26 25 16 15 0
* +--+-------+----------------+--------------------------+
- * |1 | 0 | type | code | standard OpenFlow
+ * | 1| 0 | type | code | standard OpenFlow
* +--+-------+----------------+--------------------------+ error
*
* 30 29 26 25 16 15 0
--
1.7.1
>From 90ed67c994ea51bd2db89696bf05f3a52dd45bd6 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Thu, 4 Nov 2010 10:32:46 -0700
Subject: [PATCH 2/4] ofp-util.h: Use NX_VENDOR_ID instead of literal numeric constant.
---
lib/ofp-util.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 1c683ec..c5a1897 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -95,7 +95,7 @@ char *ofp_match_to_literal_string(const struct ofp_match *match);
#define OFPUTIL_VENDORS \
/* vendor name vendor value */ \
OFPUTIL_VENDOR(OFPUTIL_VENDOR_OPENFLOW, 0x00000000) \
- OFPUTIL_VENDOR(OFPUTIL_VENDOR_NICIRA, 0x00002320)
+ OFPUTIL_VENDOR(OFPUTIL_VENDOR_NICIRA, NX_VENDOR_ID)
/* OFPUTIL_VENDOR_* definitions. */
enum ofputil_vendor_codes {
--
1.7.1
>From 73d5a5f3072d858b04fc72733ad119ca864ce9a9 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Thu, 4 Nov 2010 10:33:08 -0700
Subject: [PATCH 3/4] ofp-util: Actually map vendor codes to IDs correctly.
Reported-by: Justin Pettit <jpettit at nicira.com>
---
lib/ofp-util.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 9dc5b2a..c95e7c6 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -829,6 +829,8 @@ vendor_code_to_id(uint8_t code)
{
switch (code) {
#define OFPUTIL_VENDOR(NAME, VENDOR_ID) case NAME: return VENDOR_ID;
+ OFPUTIL_VENDORS
+#undef OFPUTIL_VENDOR
default:
return UINT32_MAX;
}
--
1.7.1
>From e05dc3a1eb12458e97a9a0602563782cb7b27f18 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp at nicira.com>
Date: Thu, 4 Nov 2010 10:33:28 -0700
Subject: [PATCH 4/4] ofp-util: Use our usual variable naming convention in make_ofp_error_msg().
---
lib/ofp-util.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index c95e7c6..261c67a 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -888,7 +888,7 @@ make_ofp_error_msg(int error, const struct ofp_header *oh)
oem->code = htons(code);
} else {
struct ofp_error_msg *oem;
- struct nx_vendor_error *ove;
+ struct nx_vendor_error *nve;
uint32_t vendor_id;
vendor_id = vendor_code_to_id(vendor);
@@ -898,15 +898,15 @@ make_ofp_error_msg(int error, const struct ofp_header *oh)
return NULL;
}
- oem = make_openflow_xid(len + sizeof *oem + sizeof *ove,
+ oem = make_openflow_xid(len + sizeof *oem + sizeof *nve,
OFPT_ERROR, xid, &buf);
oem->type = htons(NXET_VENDOR);
oem->code = htons(NXVC_VENDOR_ERROR);
- ove = ofpbuf_put_uninit(buf, sizeof *ove);
- ove->vendor = htonl(vendor_id);
- ove->type = htons(type);
- ove->code = htons(code);
+ nve = ofpbuf_put_uninit(buf, sizeof *nve);
+ nve->vendor = htonl(vendor_id);
+ nve->type = htons(type);
+ nve->code = htons(code);
}
if (len) {
--
1.7.1
More information about the dev
mailing list