<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 24, 2013 at 1:58 PM, Ben Pfaff <span dir="ltr"><<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">experimenter IDs<br>
> > by adding new *_VENDOR_ID definitions to openflow-common.h.<br>
> ><br>
> > Signed-off-by: Ben Pfaff <<a href="mailto:blp@nicira.com">blp@nicira.com</a>><br>
<br>
</div>Please snip more when you add inline comments, so that I don't<br>
accidentally miss some of your questions among the quoted text.<br>
Thanks.</blockquote><div><br></div><div style>Sorry, will starting doing it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">> > + if enum in reverse[version]:<br>
> > + error("%s: %s in OF%s means both %#x,%d,%d and "<br>
> > + "%#x,%d,%d." %<br>
> > + (dst, enum, version_reverse_map[version],<br>
> > + reverse[version][enum][0],<br>
> > + reverse[version][enum][1],<br>
> > + reverse[version][enum][2],<br>
> > + vendor, type_, code))<br>
> > + reverse[version][enum] = (vendor, type_, code)<br>
> ><br>
><br>
><br>
><br>
> Want to ask here, I'm not sure whether "if enum in reverse[version]" will<br>
> ever be true. Because, if it is true, that means there are two exactly same<br>
> enum strings, which is impossible. The compiler will complain if two<br>
> entries in the enumeration have same name.<br>
<br>
</div>I think you're right.<br>
<br>
Here's an incremental improvement, then:<br></blockquote><div><br></div><div><br></div><div style>Thanks, I'll check the new patch.</div><div style> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">> > OFPERR_OFPRRFC_BAD_ROLE,<br>
><br>
><br>
><br>
> For NX, "(1,513)", do the type and code here correspond to "struct<br>
> nx_vendor_error 's type and code"?<br>
<br>
</div>Yes.<br>
<div class="im"><br>
> What is the rule of defining NX code here?<br>
<br>
</div>The "role" feature is a Nicira extension to OpenFlow that was adopted<br>
in OpenFlow 1.2 as a standard feature, so OpenFlow 1.2 and later have<br>
standard error codes for the feature.<br>
<div class="im"><br>
> Also, seems to me that now, the legit vendors (with assigned vendor id) can<br>
> freely define their own error messages, right?<br>
<br>
</div>Yes, and in fact if you add another vendor ID to<br>
include/openflow/openflow-common.h then you can also add corresponding<br>
error codes in lib/ofp-errors.h.<br>
<div class="im"><span style="color:rgb(34,34,34)"></span></div></blockquote><div><br></div><div><br></div><div style>Thanks, the explanation makes sense.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><span style="color:rgb(34,34,34)"> </span><br></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> > dnl Write-Metadata duplicated.<br>
> > -# bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER<br>
> > +# bad OF1.1 instructions: NXBIC_DUP_INSTRUCTION<br>
> > 0002 0018 00000000 fedcba9876543210 ff00ff00ff00ff00 0002 0018 00000000<br>
> > fedcba9876543210 ff00ff00ff00ff00<br>
><br>
><br>
> Want to ask what does NXBIC stand for? Also, I didn't find the definition<br>
> of this macro, even before applying patch 5/5. Want to know how and where<br>
> is it defined?<br>
<br>
</div>Oops. This is a mistake. This change should not be in this patch.<br>
I've removed it.</blockquote><div><br></div><div><br></div><div style>I'm still curious why the test still passes. Could you explain more? Thanks</div><div style><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5">> followed the code<br>
> and decoded it myself. And I really want to ask an unrelated question.<br>
><br>
> I saw in the "ofperr_encode_msg__()" function in "lib/ofp-error.c", the<br>
> fields are converted to<br>
> network order, like below:<br>
><br>
> "<br>
> oem = ofpbuf_put_uninit(buf, sizeof *oem);<br>
> oem->type = htons(NXET_VENDOR);<br>
> "<br>
><br>
> In the "ovs_hex_dump()" function which prints the encoded header, it uses<br>
> "fprintf(stream, "%02hhx"<br>
> to print it and the value is again in host order (if %x is used, it will<br>
> print the network order).<br>
><br>
> So I want to ask what is the use of "hhx" here? and why can't we get rid of<br>
> htons and use %x in<br>
> ovs_hex_dump?<br>
<br>
</div></div>'oem' is a "wire format" type, and its member 'type' is an ovs_be16,<br>
so to appropriately assign it a value it must be converted to network<br>
byte order first. That is why htons() is used.<br>
<br>
ovs_hex_dump() dumps the contents of a structure byte by byte. Its<br>
output will depend on the byte order of the underlying data. In this<br>
case the data is in network byte order, so the byte-by-byte dump will<br>
reflect that.<br>
<br>
'hh' doesn't have anything to do with byte order. It means that the<br>
argument is a char type.<br></blockquote><div> </div><div><br></div><div style>Thanks for the explanation. My previous experiment was wrong. And I found out that the if "htons" is not used the "0xb0c2" when converted to "uint8 buf[2]" will be "buf[0]=0xc2" and "buf[1]=0xb0". So I think the "htons" also helps preserve the hex order. right?</div>
<div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'll repost this series since it has changed a bit.<br>
</blockquote></div><br></div></div>