[ovs-dev] [ext-260 v3 4/5] ofp-errors: Implement OpenFlow 1.2+ experimenter error codes.

Ben Pfaff blp at nicira.com
Mon Jun 24 20:58:51 UTC 2013


On Thu, Jun 20, 2013 at 10:37:08PM -0700, Alex Wang wrote:
> On Wed, Jun 12, 2013 at 11:35 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > OpenFlow 1.2 standardized experimenter error codes in a way different from
> > the Nicira extension.  This commit implements the OpenFlow 1.2+ version.
> >
> > This commit also makes it easy to add error codes for new experimenter IDs
> > by adding new *_VENDOR_ID definitions to openflow-common.h.
> >
> > Signed-off-by: Ben Pfaff <blp at nicira.com>

Please snip more when you add inline comments, so that I don't
accidentally miss some of your questions among the quoted text.
Thanks.

> > -                    if enum in reverse[target]:
> > -                        error("%s: %s in %s means both %d,%d and %d,%d." %
> > -                              (dst, enum, target,
> > -                               reverse[target][enum][0],
> > -                               reverse[target][enum][1],
> > -                               type_, code))
> > -                    reverse[target][enum] = (type_, code)
> > +                if enum in reverse[version]:
> > +                    error("%s: %s in OF%s means both %#x,%d,%d and "
> > +                          "%#x,%d,%d." %
> > +                          (dst, enum, version_reverse_map[version],
> > +                           reverse[version][enum][0],
> > +                           reverse[version][enum][1],
> > +                           reverse[version][enum][2],
> > +                           vendor, type_, code))
> > +                reverse[version][enum] = (vendor, type_, code)
> >
> 
> 
> 
> Want to ask here, I'm not sure whether "if enum in reverse[version]" will
> ever be true. Because, if it is true, that means there are two exactly same
> enum strings, which is impossible. The compiler will complain if two
> entries in the enumeration have same name.

I think you're right.

Here's an incremental improvement, then:

diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
index 5378dac..a34fc9d 100755
--- a/build-aux/extract-ofp-errors
+++ b/build-aux/extract-ofp-errors
@@ -257,6 +257,8 @@ def extract_ofp_errors(fn):
             fatal("syntax error expecting enum value")
 
         enum = m.group(1)
+        if enum in names:
+            fatal("%s specified twice" % enum)
 
         comments.append(re.sub('\[[^]]*\]', '', comment))
         names.append(enum)
@@ -325,14 +327,7 @@ def extract_ofp_errors(fn):
                     domain[version][vendor][type_][code] = (enum, fileName,
                                                    lineNumber)
 
-                if enum in reverse[version]:
-                    error("%s: %s in OF%s means both %#x,%d,%d and "
-                          "%#x,%d,%d." %
-                          (dst, enum, version_reverse_map[version],
-                           reverse[version][enum][0],
-                           reverse[version][enum][1],
-                           reverse[version][enum][2],
-                           vendor, type_, code))
+                assert enum not in reverse[version]
                 reverse[version][enum] = (vendor, type_, code)
 
     inputFile.close()

> > -    /* NX1.0(1,513), NX1.1(1,513), OF1.2+(11,2).  Invalid role. */
> > +    /* NX1.0-1.1(1,513), OF1.2+(11,2).  Invalid role. */
> >      OFPERR_OFPRRFC_BAD_ROLE,
> 
> 
> 
> For NX, "(1,513)", do the type and code here correspond to "struct
> nx_vendor_error 's type and code"?

Yes.

> What is the rule of defining NX code here?

The "role" feature is a Nicira extension to OpenFlow that was adopted
in OpenFlow 1.2 as a standard feature, so OpenFlow 1.2 and later have
standard error codes for the feature.

> Also, seems to me that now, the legit vendors (with assigned vendor id) can
> freely define their own error messages, right?

Yes, and in fact if you add another vendor ID to
include/openflow/openflow-common.h then you can also add corresponding
error codes in lib/ofp-errors.h.

> >  dnl Write-Metadata duplicated.
> > -# bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER
> > +# bad OF1.1 instructions: NXBIC_DUP_INSTRUCTION
> >  0002 0018 00000000 fedcba9876543210 ff00ff00ff00ff00 0002 0018 00000000
> > fedcba9876543210 ff00ff00ff00ff00
> 
> 
> Want to ask what does NXBIC stand for? Also, I didn't find the definition
> of this macro, even before applying patch 5/5. Want to know how and where
> is it defined?

Oops.  This is a mistake.  This change should not be in this patch.
I've removed it.


> > +AT_SETUP([encoding experimenter errors])
> > +AT_KEYWORDS([ofp-print ofp-errors])
> > +AT_CHECK(
> > +  [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0100000812345678],
> > [0], [dnl
> > +00000000  01 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> > +00000010  00 01 02 03 01 00 00 08-12 34 56 78 @&t@
> > +])
> > +AT_CHECK(
> > +  [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0200000812345678],
> > [0], [dnl
> > +00000000  02 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> > +00000010  00 01 02 03 02 00 00 08-12 34 56 78 @&t@
> > +])
> > +AT_CHECK(
> > +  [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0300000812345678],
> > [0], [dnl
> > +00000000  03 01 00 18 12 34 56 78-ff ff 00 04 00 00 23 20 @&t@
> > +00000010  03 00 00 08 12 34 56 78-
> > +])
> > +AT_CLEANUP
> 
> 
> I'm very curious about the en/decoding implementation here. So, I actually
> followed the code
> and decoded it myself. And I really want to ask an unrelated question.
> 
> I saw in the "ofperr_encode_msg__()" function in "lib/ofp-error.c", the
> fields are converted to
> network order, like below:
> 
> "
>         oem = ofpbuf_put_uninit(buf, sizeof *oem);
>         oem->type = htons(NXET_VENDOR);
> "
> 
> In the "ovs_hex_dump()" function which prints the encoded header, it uses
> "fprintf(stream, "%02hhx"
> to print it and the value is again in host order (if %x is used, it will
> print the network order).
>
> So I want to ask what is the use of "hhx" here? and why can't we get rid of
> htons and use %x in
> ovs_hex_dump?

'oem' is a "wire format" type, and its member 'type' is an ovs_be16,
so to appropriately assign it a value it must be converted to network
byte order first.  That is why htons() is used.

ovs_hex_dump() dumps the contents of a structure byte by byte.  Its
output will depend on the byte order of the underlying data.  In this
case the data is in network byte order, so the byte-by-byte dump will
reflect that.

'hh' doesn't have anything to do with byte order.  It means that the
argument is a char type.

I'll repost this series since it has changed a bit.



More information about the dev mailing list