[Gen-art] Gen-ART: Re-review of draft-ietf-p2psip-base-23

[Mary Barnes <[e-mail]>]

Russ asked that I re-review the updated document.

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-p2psip-base-23
Reviewer:  Mary Barnes
Review Date: 14 December 2012
Re-Review started Date (-21):  28 March 2012 (was told a new version
was coming out)
Original Review Date (-17) : 6 August 2011
IETF LC End Date: 22 July 2011

Summary: Not Ready.

I reviewed through section 6 of this version (section 5 of -17)
against my comments on the -17 . Since many of my comments had not
been addressed, I'm not going to take the time to re-review the
remainder of the document.

I also *strongly* recommend that you ensure Henning has reviewed this
document *before* it gets into the RFC editor's queue.  He has fairly
strict (and quite accurate) views on grammar and structure but it
really isn't good to have so many changes go in at AUTH48 as there is
a risk of introducing true technical bugs or changing something that
was carefully crafted to achieve WG consensus:
http://www.cs.columbia.edu/~hgs/etc/writing-bugs.html


Comments:
=========

I have reviewed my previous comments on this document -17 as compared
to the -23.  Comments from the -17 that remain are prefaced with [-17]

Major:
------

General:
--[-17] The message names are used inconsistently.  The IANA registry
has the message codes all lower case.  There are places in the
examples and the body of the document where the _req part is missing,
there is no "_", the first letters are Upper case, etc.  The messages
are also used in a general sense - e.g., "MUST perform an Attach",
which should really be stated as "MUST send an attach_req message).
[-23]  For the latter point, Cullen responded that this isn't really
just talking about sending a message but rather performing a function
that results in a message being sent.

However, I still contend that there is inconsistency and the whole
concept of an Attach function/process/whatever is not well defined.
There are times when "attached" appears to be used purely as a verb
rather than referring to the functionality, but it is not always
clear.  For example, the last two sentences in section 3.4:
   "Instead, we use the Attach request to establish a
   connection.  Attach uses ICE [RFC5245] to establish the connection.
   It is assumed that the reader is familiar with ICE."
I will note that the only mention of "Attach" previously was in the
definitions for Connection Table ("Attach handshakes") and Routing
Table ("some peers will have Attached").  The word "Attach" is used
with the _req or the word "request" in places like the following in
section 3.4 (1st sentence):   "There are two cases where Attach is not
used."  I have no idea really whether this means this Attach
function/process is not needed or the message is not required to be
sent.

Major - detailed:

- Section 3.4, last paragraph:  Accepting Cullen's comment that this
text is not normative, I'll let my first set of -17 comments on this
one go.  However, the following is still not clear to me:
What is meant by the "specified link set" - is that referring to all
the nearby peers or the peers+enough others?  Or is this more clearly
(and normatively) specified elsewhere, such that a reference could be
added.

- [-17 5.1.2] Section 6.1.2:
-- Last paragraph (9) mixes normative text with in an example. The
normative text needs to be separated from the example - e.g., split
some of the sentences into the normative behavior and then the
example. The example should be written in terms of what happens (which
is what was done in the previous examples in this section and not what
SHOULD, MAY or MUST happen).      For example, this sentence:
   "Node A MUST implement an algorithm that ensures that
   A returns a response to this request to node B with the destination
   list [B, Z, Y, X], provided that the node to which A forwards the
   request follows the same contract.
should have a generic normative statement rather than saying "Node A
MUST".  It should be something like: "A node must implement an
algorithm that ensures it returns a response …", then the same
sentence can be a "For example, Node A MUST…"

- [-17 5.3.2.1] Section 6.3.2.1:
--1st paragraph: shouldn't this be normative?
"the configuration document has a sequence number..." ->  "the
configuration document MUST contain a sequence number which MUST be
monotonically increasing mod 65535"

- [-17 5.3.2.2] -Section 6.3.2.2:
--Paragraph after second structure, suggest the following changes:
---"the generation counters should" ->  "the generation counters SHOULD"
---"An implementation could use.." -> "An implementation MAY use…"
---"the node confirms that the generation counter matches" -> "the
node MUST confirm that the generation counter matches"
---"If it does not match, it is silently dropped." -> "If it does not
match, it MUST be silently dropped."

- [-17 5.3.3.1] Section 6.3.3.1:
--1st para: "a request returns" -> "a request MUST return"
--1st para: "then the message code is the response code that matches
the request" ->
"then the message code MUST be set to the response code that matches
the request"
--2nd para: "message code is set" ->  "message code MUST be set"

- [-17 5.3.4] Section 6.3.4 - needs normative language:
--1st paragraph after GenericCertificate structure:
---"All signatures are formatted" -> "Alls signatures MUST be formatted"
---"The input structure…varies.." ->  "The input structure … MAY vary…"

- [-23] Review stops at section 6.4.2


Minor:
------
- idnits identifies 5 errors (downrefs).  There are also unused
references and 22 warnings that should be reviewed - the ones, in
particular, which should be fixed are with regards to normative
language usage.

- While Cullen responded to this comment with an explanation, there
was no change to clarify the text:
- [-17] Section 1.2.1, 2nd paragraph: I don't understand the example
as to why a single application requires multiple usages - i.e, why
voicemail?  Isn't the intent to say that an application might need to
use both SIP and XMPP - i.e., you wouldn't define a "usage" for an
application, would you?

- [-17] Section 2. Some of the definitions use terms that are not yet
defined, so that's not particularly helpful - i.e,. Attach and Update
are used. In some cases (e.g., Connection Table), you could just
delete the statement with the reference.

- [-17] Section 3.1, 1st para: This sentence doesn't grok because the
concept of "overlay parameters" hasn't been mentioned before this
section nor has the concept of a "configuration document".
  "The overlay parameters are specified in a configuration document."
At a minimum those terms should be defined in the terminology section
and introduced in the intro paragraph in section 3.

- [-17] Section 3.3, last paragraph.  Add a reference to 5.4.2.4
after "RouteQuery method"

- [-17] Section 5.1.2:
--1st paragraph: I have no idea what the "other three cases" references.

- [-17 Section 5.3] Section 6.3:  The three parts of the messages are
listed and the last sentence says that "The following sections
describe the format of each part of the message", but there are 4
sub-sections in 5.3, with the first section NOT describing one of the
three parts of the messages.  I suggest to strike that last sentence
and just put references to the sections in the list items for each of
the three messages

- [-17 Section 5.6] Section 6.6:
-- The last three paragraphs are confusing.
---In particular, it's not clear what algorithms are being referenced
in this sentence:
  "We will first define each of these algorithms, then
   define overlay link protocols that use them."
--- In the note paragraph, I believe "These protocols have been
chosen…" is referring to the three overlay link protocols supported by
RELOAD per this document.
---The Note to implementors paragraph seems out of context
--- Per my next comment, I suggest to move the detailed discussion of
future overlay link protocols to the appendix, so some of this text
may be relevant only to that.

-[ -17 5.6.1] Section 6.6.1:  I suggest that the sub-sections in this
section be moved to an appendix and a reference to such be added.

[-23] Did not review beyond section 6.

Nits:
-----

- [-17] Section 2.
--General: It would be helpful if the terms were alphabetized.
[Note, Cullen's response was that this causes forward reference
problems, but that's unavoidable. If the terms are alphabetized, it is
much easier to find them. ]
--Node: "We use the term "Node" to refer to …" -> "Refers to …" or
something to make it more consistent with the format for the other
terms.

- Section 6.1.1, last bullet:  "later" -> "latter"

- Section 6.1.3: Shouldn't this be "For example," rather than "I.e.,"?
 If "I.e.," is appropriate in that those are the only routing
algorithms which might be used, then I would suggest adjusting the
sentence structure by using words rather than the abbreviation (it's
debatable whether the latter is grammatically correct) to something
like.  "This means that a node…".

- [-17 5.5.1.6] Section 6.5.1.6: Add references for SCTP and DCCP.

[-23] Did not review beyond nits identified for section 6.