Subject: [P2PSIP] Review of draft-ietf-p2psip-diagnostics-06

From: Marc Petit-Huguenin <[email-masked]>
Date: Sun, 06 Nov 2011 08:29:34 -0800

1. Section 5.1.1, structure definition

The structure in the document is mangled, but should not this be better:

enum { (2^16-1) } DiagnosticExtensionRequestType;

struct {
  DiagnosticExtensionRequestType type;
  opaque diagnostic_extension_contents<0..2^32-1>;
} DiagnosticExtension; // the ";" was missing

struct {
  uint64 expiration;
  uint64 timestampInitiated;
  uint64 dMFlags;
  DiagnosticExtension diagnostic_extensions<0..2^32-1>;  // the length is
already here.
} DiagnosticsRequest;

Also the authors should choose between the CamelCase convention
(timestampInitiated) or the underscore convention
(diagnostic_extension_contents) but not mix the two.

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?

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?

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.

5. Section 5.1.3, MEMORY_FOOTPRINT

A "kilo byte" of 1024 bytes is called kibibyte.

6. Section 5.3.1: structure definition

The request type should be DiagnosticsRequest instead of DiagnosticRequest.

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).

For error code 101, why using a fixed-length text string, instead of a "opaque
suberror<0..32>"

8. Section 8.1

This section should define the name of the new IANA registry

9. Section 8.2

This section should define the name of the new IANA registry

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/

- -- 
Marc Petit-Huguenin
Personal email: [email-masked]
Professional email: [email-masked]
Blog: http://blog.marc.petit-huguenin.org

---

From: Songhaibin <[email-masked]>
Date: Fri, 11 Nov 2011 07:03:48 +0000

Hi Marc,

Thank you very much for the review. See reply in line.

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] 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
> 
Thank you for catching it. I will correct it.

> } DiagnosticExtension; // the ";" was missing

OK.

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. 

> (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.

> used by RELOAD (milliseconds since the epoch).  Is there really a need for

If there is a consensus to use Unix time, the diagnostic draft will be updated to be consistent.

> 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.

> 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?

> A "kilo byte" of 1024 bytes is called kibibyte.

That's really good. With "kibibyte" we do not need an extra explanation here:)

> The request type should be DiagnosticsRequest instead of DiagnosticRequest.

Good catch.

> 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

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.

> This section should define the name of the new IANA registry

Yes. I will add it.

> 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.

> s/Ping message or with/Ping message with/

Thank you again, Marc.

-Haibin

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> 
> iEYEARECAAYFAk62tewACgkQ9RoMZyVa61e/3ACgqlLAmnDTLZGG78QU0kT8shzL
> p0kAnjEhQ741c+07Gn5gAhY6j4j+FtGY
> =i+f1
> -----END PGP SIGNATURE-----
> _______________________________________________
> P2PSIP mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/p2psip

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Fri, 11 Nov 2011 10:21:53 -0500

Inline.

On 11/11/2011 02:03 AM, Songhaibin wrote:
>> -----Original Message----- From: [email-masked]
>> [mailto:[email-masked]] 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; 
> 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.

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

Really?  I would have thought that it was the size, not the number of instances.

>> 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;
> 

Yes, but my point was that timestampInitiated is not consistent with either the
base draft or the other fields in this draft.

[...]

>> 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?

> 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.
> 
> 

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.

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.

[...]

> 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>

- -- 
Marc Petit-Huguenin

---

From: Songhaibin <[email-masked]>
Date: Tue, 15 Nov 2011 00:12:49 +0000

Hi Marc,
________________________________________
From: [email-masked] [[email-masked]] on behalf of Marc Petit-Huguenin [[email-masked]]
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.

[...]

>> c} }

[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]". 

[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...

[...]

[Haibin] If my understanding is correct, the answer is yes. It would include the types that are to be defined in the future. 

[Haibin] I'm okay with adding OS info.

[Haibin] I think this suggestion is good.

[...]

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

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Tue, 15 Nov 2011 08:27:30 +0800

On 11/15/2011 08:12 AM, Songhaibin wrote:
> Hi Marc, ________________________________________ From:
> [email-masked] [[email-masked]] on behalf of Marc
> Petit-Huguenin [[email-masked]] 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.
> 
>>> 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.
> 
> 
> [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];

[...]

>>> 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.

>>> 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.
> 

Thanks.

> [Haibin] I think this suggestion is good.

Great, thanks.

> uint8  subcode; opaque reason<0..2^8-1>
> 

Ah, sorry! I'll try to find in the mailing-list if there was any discussion
explaining this.

Thanks.

- -- 
Marc Petit-Huguenin

---

From: Songhaibin <[email-masked]>
Date: Fri, 25 Nov 2011 07:44:28 +0000

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

> Songhaibin
> Sent: Tuesday, November 15, 2011 8:13 AM
> To: Marc Petit-Huguenin
> Cc: P2PSIP Mailing List
> number_diag_ext.  And "DiagnosticExtension diagnostic_extensions<0..2^32-1>
> " will be changed to "DiagnosticExtension
> [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...
> 
> 
> [...]
> 
> >
> [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
> >
> iEYEARECAAYFAk69PY4ACgkQ9RoMZyVa61eu4gCdEJkyC9YsRpCPutdBLlX6ayB5
> xPMAn1PC0oNgCGt0w0ABMMpoMK8zNN9F
> =s463