-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 11/15/2011 08:12 AM, Songhaibin wrote: > 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 > > 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]".
That would still redefine the meaning of [], which would be confusing for implementers. What I would suggest is to use the length, and to define this: DiagnosticExtension diagnostic_extensions[length]; [...] > > >> 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. > Thanks. > > >> 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. Thanks. > > 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. Great, thanks. > > [...] > >> 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. Ah, sorry! I'll try to find in the mailing-list if there was any discussion explaining this. Thanks. - -- 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) iEYEARECAAYFAk7BsfAACgkQ9RoMZyVa61d2RQCfcAzc21npImYh/lVUeMYzyfHz VmEAniGbJ7kUv/3UNTjFbVK3u1p+scW0 =HrOD -----END PGP SIGNATURE----- _______________________________________________ P2PSIP mailing list [email protected] https://www.ietf.org/mailman/listinfo/p2psip
