Hi Marc, ________________________________________ From: [email protected] [[email protected]] on behalf of Marc Petit-Huguenin [[email protected]] Sent: Friday, November 11, 2011 11:21 PM To: Songhaibin Cc: P2PSIP Mailing List Subject: Re: [P2PSIP] Review of draft-ietf-p2psip-diagnostics-06
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Inline. On 11/11/2011 02:03 AM, Songhaibin wrote: > Hi Marc, > > Thank you very much for the review. See reply in line. > >> -----Original Message----- From: [email protected] >> [mailto:[email protected]] On Behalf Of Marc Petit-Huguenin Sent: >> Monday, November 07, 2011 12:30 AM To: P2PSIP Mailing List Subject: >> [P2PSIP] Review of draft-ietf-p2psip-diagnostics-06 [...] > > struct { uint64 expiration; uint64 timestampInitiated; uint64 dMFlags; > DiagnosticExtension diagnostic_extensions<0..2^32-1>; // the length is > already here. } DiagnosticsRequest; > >> struct { uint64 expiration; uint64 timestampInitiated; uint32 length; >> select (length) { case 0: uint64 dMFlags; case > 0: uint64 dMFlags; >> c} } >> DiagnosticsRequest; > >> By using structure, we mean that if the "length" field at the beginning of >> the request equals to "0", there is no other diagnostic information request >> except that contained in the dMFlags. If there are other extension types of >> diagnostic information are requested(as indicated by the length field), >> there will be DiagnosticEXtension field. But it seems there is something >> wrong here. Because in the draft it only allows at most 2^16 extension >> types, the diagnostic_extensions array should also be at most 2^16 long. >> Section 5.1.2 has the same problem. What I meant is that the explicit length field and the implicit length field in the diagnostic_extensions field carry the same value, so why not eliminate one of them. > >> That is to say, "length" is the number of requested extension diagnostic >> types. Really? I would have thought that it was the size, not the number of instances. [Haibin] So We could change the explicit length field to another name, say, number_diag_ext. And "DiagnosticExtension diagnostic_extensions<0..2^32-1> " will be changed to "DiagnosticExtension diagnostic_extensions[number_diag_ext]". > > > Also the authors should choose between the CamelCase convention > (timestampInitiated) or the underscore convention > (diagnostic_extension_contents) but not mix the two. > > >> That's to make it consistent with the base draft. For example, in the base >> draft section 5.3.3.1, the definition of ErrorResponse struct: > >> public struct { uint16 error_code; opaque >> error_info<0..2^16-1>; } ErrorResponse; > >> You see the structure name is in CamelCase, but the variable is in >> underscore convention. Yes, but my point was that timestampInitiated is not consistent with either the base draft or the other fields in this draft. [Haibin] Sorry. I thought you mean in the whole draft. But yes, the authors would find them in the draft and correct them to timestamp_initiated, timestamp_received, and... [...] > > 3. Section 5.1.1, third field definition > > Is the request really allowed to set all dMFlags to '1', and so requesting > diagnostics types that are not yet defined? > > >> Only defined main diagnostics types are requested. From the extensibility >> consideration, it is good to classify the diagnostics types into two, one >> is the main types, the other is the extended types. We use 64 bits dMFlags >> to indicate the main diagnostics types that are requested. If there is >> requirement for more diagnostics types, they can define the extension types >> for use. The request will then contain the diagnostic_extensions<0..2^32-1> >> field as defined in section 5.1.1. That's not what I meant. On the 64 main diagnostics types possibles, only 15 are defined. But the text says that all dMFlags must be set to '1', so it would request types that are not defined yet. Is it the intent? [Haibin] If my understanding is correct, the answer is yes. It would include the types that are to be defined in the future. > > > 4. Section 5.1.3 > > I would add an additional diagnostic kind that provide the OS (or modify > SOFTWARE_VERSION to provide the OS), for the purpose of evaluating if an > overlay as enough OS diversity to handle a bug in a specific OS. > > >> To understand it deeply, could you give an example of what a bug it might >> be and how it will impact the overlay? It's about diversity in an overlay. If all the nodes are using the same RELOAD implementation, then the overlay will not survive a bug in this implementation. So a node can use diagnostics to evaluate the diversity in software implementations. Because OS have bugs too, OS diversity in an overlay is also a good thing, but diagnostics cannot be used for this purpose. [Haibin] I'm okay with adding OS info. Which bring another thing that is missing in diagnostics: The draft should define an XML namespace for the sole purpose of adding it to a <mandatory-extension> element, to force all nodes of an overlay to support diagnostics. [Haibin] I think this suggestion is good. [...] > For error code 101, why using a fixed-length text string, instead of a > "opaque suberror<0..32>" > > >> There are several types when error 101 was returned. We want to make it >> machine readable than just human readable, so as to classify them. I think that a subcode + text with variable length is better than parsing an exact text, something like this: uint8 subcode; opaque reason<0..2^8-1> [Haibin] That's what we did in the previous draft long time ago. BR, -Haibin - -- Marc Petit-Huguenin Personal email: [email protected] Professional email: [email protected] Blog: http://blog.marc.petit-huguenin.org -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEARECAAYFAk69PY4ACgkQ9RoMZyVa61eu4gCdEJkyC9YsRpCPutdBLlX6ayB5 xPMAn1PC0oNgCGt0w0ABMMpoMK8zNN9F =s463 -----END PGP SIGNATURE----- _______________________________________________ P2PSIP mailing list [email protected] https://www.ietf.org/mailman/listinfo/p2psip _______________________________________________ P2PSIP mailing list [email protected] https://www.ietf.org/mailman/listinfo/p2psip
