Subject: [apps-discuss] [APPS-REVIEW] review of draft-ietf-p2psip-diagnostics-13

From: "Martin J. Dürst" <[email-masked]>
Date: Mon, 19 May 2014 19:07:19 +0900

I have been selected as the Applications Area Review Team reviewer for 
this draft (for background on apps-review, please see 
http://www.apps.ietf.org/content/applications-area-review-team).

Please resolve these comments along with any other Last Call comments 
you may receive. Please wait for direction from your document shepherd 
or AD before posting a new version of the draft.

Document: draft-ietf-p2psip-diagnostics
Title: P2P Overlay Diagnostics
Reviewer: Martin J. Dürst
Review Date: various days before and including May 19, 2014
IETF Last Call Date: not yet
IESG Telechat Date: not yet

Summary: To me, the draft looks in reasonable shape, with some issues to 
be fixed as bellow. However, I'm not a SIP or P2P expert, and therefore 
this review shouldn't be taken as an active endorsement of the draft.

Preface: I sincerely apologize for the long time it took me to review 
this draft. I have reviewed -13, but I have also checked the diff at 
http://tools.ietf.org/rfcdiff?url2=draft-ietf-p2psip-diagnostics-14.txt 
to make sure I haven't missed anything new.

Overall issue: There were quite a few places in the text where the 
wording choice was somewhat suboptimal. I strongly recommend a review by 
a native English speaker familiar with the subject.

Minor issues:

Structure: The meat of the draft is all in section 5. It may make sense 
to split that section (but merge others) to get a better balance. But I 
can't give a specific proposal, sorry. The start of section 5 also reads 
like an abstract for a whole document.

p. 9: DiagnosticRequest has 'dmFlags' before 'length', but in the text, 
the order is reversed. Please align.

p. 10: There is only a single 'kind' code (0xFFFE) reserved for private 
use. Given that currently only about 16 such codes are defined in the 
spec, it seems okay to reserve a somewhat larger number of these codes 
for private/experimental use.

5.1.2, timestamp_received (and timestamps in general): is a resolution 
of milliseconds good enough? It may be way more than actually useful in 
a world-wide overlay, but it could be less than desirable in a local 
deployment. Same also for the limits (10 to 600 seconds) on the 
expiration field. A local network should probably not need a timeout of 
10 seconds.

5.1.3, PROCESS_POWER, BANDWIDTH: 32bits of MIPS and Kbps seem to cover 
the current range of hardware, but what about extremely slow hardware or 
very fast hardware in a few years? Similar for bandwidth. What about 
e.g. defining some kind of log scale, which may reflect the long-term 
realities of technology and market better? Same for MEMORY_FOOTPRINT.
5.1.3, SOFTWARE_VERSION: there is a '*', is that supposed to be some 
kind of syntax indicator (e.g. repetition as in regular expressions)? 
The example should be put into quotes.

5.1.3, BATTERY_STATUS: "using a battery power" -> "using battery power"

5.2: "and will treat the message if it was a normal ping" -> "and will 
treat the message *AS* if it was a normal ping"

5.4: I'm surprised that this diagnostics document has to define an error 
code for "Underlay Destination Unreachable". Shouldn't such an error 
condition occur in the base protocol already?

5.5.1: There are way too many MUSTs here. It would be much better to put 
limitations on field values where these field values are defined, and 
just describe the limitations in positive language.

5.5.1: The first paragraph uses "Ping message with diagnostic 
extension", the second paragraph uses "diagnostic Ping message". Please 
unify terminology.

5.5.3: The first two paragraphs semantically form one single sentence. 
The first paragraph forms one syntactical sentence. Both of these are 
too long.

In the first paragraph, it is impossible to understand to what clauses 
the initial "When" applies without rereading the paragraph. Please fix.

Section 6: Just to check, would this be what's described in this section:

<diagnostic-kind kind='0006'>  <!-- MACHINE_UPTIME -->
   <access-node><!-- hex NodeID goes here --></access-node>
   <access-node><!-- another hex NodeID may go here --></access-node>
   <!-- more access-node elements as needed -->
</diagnostic-kind>

Would <diagnostic-kind kind='6'> also be okay? Or should it be 
<diagnostic-kind kind='0x0006'>? Things like these should be carefully 
specified. An actual example also will help.

8.1 and 8.5: Rather than creating two registries that have to be kept in 
sync, it would be better to create one registry where the first 64 
entries have more columns than the rest.

8.6: "XML namespaces": I only see one namespace. Is the intent that the 
example above actually be written e.g. like
<diagnostic-kind kind='0006'
     xmlns='urn:ietf:params:xml:ns:p2p:config-diagnostics'>
   <access-node><!-- hex NodeID goes here --></access-node>
   <access-node><!-- another hex NodeID may go here --></access-node>
   <!-- more access-node elements as needed -->
</diagnostic-kind>

Why is there a need for a separate namespace for just one or two 
elements? It's a common misconception that each spec that defines a bit 
of XML has to have its own namespace. As long as there is coordination 
to avoid element name collisions (which should be achievable e.g. in an 
IETF WG), there's absolutely no need for such piecemeal namespaces.

<mandatory-extension>: I'm not at all sure it's a good idea to use an 
XML namespace URI to identify protocol/format extensions. But it may 
already be too late to fix this.

Regards,   Martin.