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 > > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > 1. Section 5.1.1, structure definition > > The structure in the document is mangled, but should not this be better: > Thank you for catching it. I will correct it. > enum { (2^16-1) } DiagnosticExtensionRequestType; > > struct { > DiagnosticExtensionRequestType type; > opaque diagnostic_extension_contents<0..2^32-1>; > } DiagnosticExtension; // the ";" was missing > OK. > 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; DiagnosticExtension diagnostic_extensions<0..2^32-1>; } } 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. That is to say, "length" is the number of requested extension diagnostic types. > > 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. > > 2. Section 5.1.1, first and second field definitions > > This fields uses the NTP timestamp format, which is different from the format > used by RELOAD (milliseconds since the epoch). Is there really a need for > microseconds here? > If there is a consensus to use Unix time, the diagnostic draft will be updated to be consistent. > > 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. > > 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? > 5. Section 5.1.3, MEMORY_FOOTPRINT > > A "kilo byte" of 1024 bytes is called kibibyte. > That's really good. With "kibibyte" we do not need an extra explanation here:) > > 6. Section 5.3.1: structure definition > > The request type should be DiagnosticsRequest instead of DiagnosticRequest. > Good catch. > > 7. Section 5.4 > > This section should define the content of error_info for error codes 102 to > 106 > (probably UTF-8 text, but -base does not explicitly says what the default > format > is for error_info, so it need to be explicitly defined in each extension). > Good. They should be opaque text. > 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. > > 8. Section 8.1 > > This section should define the name of the new IANA registry > Yes. I will add it. > > 9. Section 8.2 > > This section should define the name of the new IANA registry > These Kind-IDs will be registered to the registry defined in Section 13.6 in the base draft. > > Nits > - ---- > > - - Section 1 > > s/compliment/complement/ > > - - Section 3 > > s/from the overlay ./from the overlay./ > > - - Section 4.1 > > s/the RELOAD ping/the RELOAD Ping/ > > - - Section 4.3 > > s/-- one/- one/ > > - - Section 5.1 > > s/are defined. DiagnosticRequest/are defined, DiagnosticRequest/ > > - - Section 5.3 > > s/New Reqeust/New Request/ > > - - Section 5.1.1, first paragraph > > s/Ping message or with/Ping message with/ > Thank you again, Marc. -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) > > iEYEARECAAYFAk62tewACgkQ9RoMZyVa61e/3ACgqlLAmnDTLZGG78QU0kT8shzL > p0kAnjEhQ741c+07Gn5gAhY6j4j+FtGY > =i+f1 > -----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
