Marc, >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.
Just noticed that you are right here. I will make the change. BR, -Haibin > -----Original Message----- > From: [email protected] [mailto:[email protected]] On Behalf Of > Songhaibin > Sent: Tuesday, November 15, 2011 8:13 AM > To: Marc Petit-Huguenin > Cc: P2PSIP Mailing List > Subject: Re: [P2PSIP] Review of draft-ietf-p2psip-diagnostics-06 > > 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 _______________________________________________ P2PSIP mailing list [email protected] https://www.ietf.org/mailman/listinfo/p2psip
