On Thu, Jun 20, 2013 at 10:37:08PM -0700, Alex Wang wrote:
> On Wed, Jun 12, 2013 at 11:35 AM, Ben Pfaff <[email protected]> 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 <[email protected]>
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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
