I'm resending to try to capture one additional author (Henning) that bounced
and the Gen-ART mailing list.  I could not find a valid email for Salman
Basset  Please reply to this email.

Thanks,
Mary.

On Tue, Aug 9, 2011 at 6:05 PM, Mary Barnes <mary.ietf.barnes@gmail.com>wrote:

>
> 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>.
>
> Sorry for the delay, but a two week IETF-LC for a 150 page document two
> weeks before an IETF meeting virtually ensures that the document  won't be
> carefully reviewed by the community and that directorate reviews will are
> late ;)   While this review was done specifically on the -17, I have
> reviewed the diff and these comments still apply to the -18.
>
> =======================================================================
>
> Document: draft-ietf-p2psip-base-17
> Reviewer:  Mary Barnes
> Review Date:  9 August  2011
> IETF LC End Date: 22 July 2011
>
> Summary: Not Ready.
>
> Comments:
> ----------------
> The document is very a dense (with detailed technical information) and long
> (150 page) specification for a Peer-to-peer signaling protocol.  While the
> overall technical functionality seems fairly correct and thoroughly
> specified, the primary issue is a tremendous lack of normative language in
> the main body of the document.  Non-inclusive details of such are included
> below.
>
> Major Issues:
> -------------------
>
> General:
> --There is a fair number of cases where normative language should be used.
>  I have detailed some (but certainly not all)  instances where it is lacking
> with proposed rewording.  I have also noted cases where it is used (lower
> case) where it could be confused with needing normative language and where
> it isn't necessary and there are some cases where it's just not clear. I
> consider these major since it can impact interoperability and could lead
> developers to not implement key functionality.
>
> --There is also little or no normative language for the majority of the
> elements in the structures - i.e., the reader has to assume which elements
> are mandatory and which are optional and it's not always clear.   For
> example, rather than saying "Field x contains blah", it should say "Field x
> MUST contain blah" and rather than "The contents of the message will be x, y
> and z", it should say something like "The message MUST (or SHOULD) contain
> x, y and z".  I have not detailed the places where this is necessary in most
> cases.  The authors should review the detailed definition for each of the
> messages.
>
> --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)
>
> Major - Detailed:
>
> - Sections 3.2.1, 3.2.2 and 3.3: lots of changes needed to reword using
> normative language.
>
> - Section 3.4, last paragraph: Needs to be reworded normatively and more
> precisely. It's really not clear to me what the required behavior is as it
> first says it needs to (which I'm not sure is a SHOULD or is REQUIRED to)
> maintain connections to all the peers near and enough others.., but how much
> is "enough".   Also, 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.
>
> - Section 4.2: needs normative language - i.e.:
> --"Each Usage needs to …" ->  "Each Usage MUST…"
>
> - Section 5.1.1: Bullet items need normative language
> -- 1st bullet: It's also  not clear what the its are in "it passes it up".
>  I think it should be "the node passes the message up to the upper layers".
>   However, this probably needs to be normative language - i.e., "the node
> MUST pass the message to the upper layers".  The same applies to the third
> bullet.
> --bullet 2:  "the message is destined for this node and is passed up…" ->
> "the message is destined for this node and MUST be passed….,
>
> - Section 5.1.2:
> -- Paragraphs 5, 6, 7 and 8 need to be reworded to have normative
> statements.
> -- 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.
>
> -Section 5.2.1 is totally inconsistent with the use of normative language
> (i.e., lower case occurrences and lack of KEY words) - shouldn't the
> following be normative statements something like the following:
> -- First paragraph:
> ---First sentence:  "a node constructs" -> "a node MUST construct".
> --- 3rd sentence: "resulting message will use the normal overlay routing"
> -> "resulting message MUST use the normal overlay routing"
> --- 4th sentence: "The node can also" ->  "The node MAY also"
> -- Third paragraph:
> --- 3rd sentence: "a single request can also" -> "a single request MAY
> also"
> -- Fourth paragraph:
> --- 1st sentence: this could be interpreted as normative:  "Because
> messages may" ->  "Because messages can"
> --- 3rd sentence:  "the request is retransmitted" ->  "the request MUST be
> retransmitted"
>
> -Section 5.3.2:
> -- General: some of the contents of the structure have normative language
> and others don't.  There is some normative behavior defined for these fields
> later, so perhaps adding section references as you do for some fields would
> be helpful.
>
> For example:
> --"overlay": shouldn't this be normative:  "the overlay name is hashed with
> SHA-1" -> "the overlay name MUST be hashed with SHA-1"
> --"destination_list": seems like some of this should be normative?
>
> -Section 5.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"
> -- 1st paragraph, last sentence: "may"  -> "can"
>
> -Section 5.3.2.2:
> --Paragraph after the 1st structure:  Shouldn't this be stated normatively
> since it is described how the structure MUST be handled depending upon the
> contents?
> --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."
>
> -Section 5.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"
>
> -Section 5.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…"
>
> -Section 5.4.1: "need to be described" -> "MUST be described"
>
> -Section 5.4.2-Section 7.  These sections need to be carefully reviewed and
> updated appropriately with normative text.  [ I have marks up for pages
> 55-102 where normative language is needed if someone wants those scanned and
> forwarded - I'm just too lazy to type them all out.]
>
> - Section 8:  There is only one actual normative word used in this section!
>  There is a lc "should" that ought to be "SHOULD".  I don't know if the
> authors assumed that the normative language in TURN was sufficient, but I do
> not believe that is the case as it leaves the normative behavior to be
> inferred by the reader/implementor. Rather than rewrite the whole section
> myself, the authors need to review carefully. For example, there are "needs
> to", which likely are really and lots of present tense statements (e.g.,
> "The address of the peer is stored…"), which ought to be MUSTs or SHOULDs.
>
> - Section 9.2:  seems like this needs normative language. Suggest the
> following:
> OLD:
>    For this Chord based topology plugin, the size of the Resource-ID is
>    128 bits.  The hash of a Resource-ID is computed using SHA-1
>    [RFC3174]then truncating the SHA-1 result to the most significant 128
>    bits.
> NEW:
>    For Chord-RELOAD, the size of the Resource-ID MUST be
>    128 bits.  The hash of a Resource-ID MUST be computed using SHA-1
>    [RFC3174].  The SHA-1 result MUST be truncated to the most significant
>    128 bits.
>
> - Section 9.3: Needs normative language.
> OLD:
>    If a peer is not responsible for a Resource-ID k, but is directly
>    connected to a node with Node-ID k, then it routes the message to
>    that node.  Otherwise, it routes the request to the peer in the
>    routing table that has the largest Node-ID that is in the interval
>    between the peer and k.  If no such node is found, it finds the
>    smallest Node-Id that is greater than k and routes the message to
>    that node.
> NEW:
>    If a peer is not responsible for a Resource-ID k, but is directly
>    connected to a node with Node-ID k, then it MUST route the message to
>    that node.  Otherwise, it MUST route the request to the peer in the
>    routing table that has the largest Node-ID that is in the interval
>    between the peer and k.  If no such node is found, it MUST find the
>    smallest Node-Id that is greater than k and MUST route the message to
>    that node.
>
> - Section 9.4: Needs normative language.
>
> - Section 9.5: While this section does have a fair amount of normative
> language, it is still lacking in a few cases:
> --Item 2.:  "should" -> "SHOULD"
> --Item 5.:  "The AP sends the response…" -> "The AP MUST send the
> response…"
> --1st paragraph after list: "…can directly connect…" -> "MAY directly
> connect"
> --2nd paragraph after list: "it can still populate" -> "it MAY still
> populate"
> --2nd paragraph after list: the description of the use of PING should be
> reworded with normative language (I'll leave that exercise to the authors)
> --3rd paragraph after the list:  "JP simply sends" -> "JP MUST send"
>
> - Section 9.7.1:
> --1st para:  ".., it then sends…" -> ".., it then MUST send …"
> --3rd para: needs normative language
> OLD:
>    In no event does
>    an attempt to reestablish connectivity with a lost neighbor allow the
>    peer to remain in the neighbor table.
> NEW:
>    In the case of
>    an attempt to reestablish connectivity with a lost neighbor, the peer
>    MUST not remain in the neighbor table.
> OR:
>    In the case of
>    an attempt to reestablish connectivity with a lost neighbor, the peer
>    MUST be removed from the neighbor table.
> -- 4th para: "should" -> "SHOULD" (2 cases),  "use Pings" -> "MUST use
> Pings"
>
> -Section 9.7.2:
> --1st para:  "..the failed peer are removed.." -> "…the failed peer MUST be
> removed.."
> --2nd para:  "…the peer initiates…" -> "…the peer MUST initiate.."
>
> -Section 9.7.3:
> --1st para after list:  "is needed" -> "is REQUIRED"
> --3rd para after list:  "the peer sends an Update" -> "the peer MUST send
> an Update"
>
> -Section 9.7.4.4:  Needs a scrub for normative language
>
> -Section 9.9: Needs a scrub for normative language.
>
> -Section 10.1: General: Needs a lot of work to be specified using normative
> language.  The majority of the configuration elements are not specified
> using normative language - for example, terms such as permitted, allowed,
> valid are used in statements rather than normative language
> or they are indicative of the need for normative language.  A specific
> example is in the definition for "clients-permitted":
>
> OLD:
>       This element represents whether clients are
>       permitted or whether all nodes must be peers.  If it is set to
>       "true" or absent, this indicates that clients are permitted.  If
>       it is set to "false" then nodes are not allowed to remain clients
>       after the initial join.
>
> NEW:
>       This element represents whether clients are
>       permitted or whether all nodes need to be peers.  If clients
>       are permitted, the element MUST be set to "true" or absent.
>       If the nodes are not allowed to remain clients after the
>       initial join, the element MUST be set to "false".
>
> --Section 10.1 - 4th para after the element list states that "All of the
> non optional values MUST be provided".  However, the list of elements in the
> previous paragraph are not specifically identified as non optional.  I think
> the first 3 are mandatory.  So, I would suggest to re-word the 3rd paragraph
> as follows and delete the 1st sentence in the 4th paragraph noted above:
> OLD:
>    In addition, the kind element contains the following elements:
>    max-count:  the maximum number of values which members of the overlay
>       must support.
>    data-model:  the data model to be used.
>    max-size:  the maximum size of individual values.
>    access-control:  the access control model to be used.
>    max-node-multiple:  This is optional and only used when the access
>       control is NODE-MULTIPLE.  This indicates the maximum value for
>       the i counter.  This is an integer greater than 0.
>
> NEW:
>    In addition, the kind element MUST contain the following elements:
>    max-count:  the maximum number of values which members of the overlay
>       must support.
>    data-model:  the data model to be used.
>    max-size:  the maximum size of individual values.
>    access-control:  the access control model to be used.
>
>    The kind element MAY also contain the following element:
>    max-node-multiple:  if the access control is NODE-MULTIPLE,
>       this element MAY be included. This indicates the maximum value for
>       the i counter.  It MUST be an integer greater than 0.
>
> --Section 10.1, next to last para: There's no normative language, but it
> seems there should be since it is specifying specification computations and
> encoding that I believe are required.
>
> - Section 10.2:  Needs more normative language:
>
> -- 2nd para:
> OLD: This value is provided by the user or some other out of band
>      provisioning mechanism.
>
> NEW: This value MUST be provided by the user or some other out of band
>      provisioning mechanism."
>
> -- 3rd para:
> OLD:
>    Once an address and URL for the configuration server is determined,
>    the peer forms an HTTPS connection to that IP address.
> NEW:
>    Once an address and URL for the configuration server is determined,
>    the peer MUST form an HTTPS connection to that IP address.
>
> -- 4th para:  "which replaces" ->  "which MUST replace"
>
> -- 5th para:  "nodes obtain the" -> "nodes MUST obtain the"
>
> - Section 10.3:
> -- 3rd para:  "is performed" -> "MUST be performed".
> -- 3rd para: Bullets need to be modified to normative.
> -- 4th para: "the certificate contains" -> "the certificate MUST contain"
> -- para after 2nd set of bullets:  needs normative language.
> -- 2nd para after 2nd set of bullets:  "The node then reads…" ->  "The node
> MUST then use…"
>
> - Section 10.5:
> --Second para: "the joining node first forms" -> "the joining node MUST
> first form"
> --3rd para:  text uses the phrase "it can note the Node-ID in the response
> and use this Node-ID to start sending requests".  It's not clear whether the
> use of the Node-ID is a MAY or a MUST.
>
> - Section 12. I'm assuming that the security directorate has reviewed this
> in detail, thus my focus in reviewing this section was on general
> understanding. The biggest issues is that there is NO normative language at
> all in the security section.  As in other sections, terms like "need",
> "are/is used/sent, etc.",  "ensure", lower case reserved words rather than
> upper case, etc. are used when normative language ought to have been used.
>
>
> - Section 13. IANA Registrations:
> - General: there seem to be several registries for which the
> usage/requirement is unclear as there's no mention of such in the body of
> the document.  In general, references to the sections in the document
> describing the information included in the registries would be more than
> helpful.
>
> - Section 13.5: Creates a RELOAD Application Registry, but the use and need
> for such isn't described elsewhere in the document.
>
> - Section 13.12. Forwarding Options Registry. SEction 5.3.2.3 seems to
> define three flags for the Forwarding options type, so it's not clear to me
> why this registry has just "invalid" and "reserved"
>
>
> - Section 13.15:  Defines/registers a RELOAD URI schema but there is no
> mention of this URI anywhere else in the document.  In what context is it
> used?
>
>
> Minor  Issues:
> ---------------------
>
> - 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?
>
> - 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.
>
> - Section 3.3, last paragraph.  Add a reference to 5.4.2.4  after
> "RouteQuery method"
>
> - Section 5.1: "If any of these are incorrect…"  - need to specify what
> qualifies as "incorrect" - is it wrong formatting, invalid header field
> value, etc.?
>
> - Section 5.1.2:
> --1st paragraph: I have no idea what the "other three cases" references.
>
> - Section 5.2: "may be configured" -> "can be configured",  "alternative
> routing algorithms may be selected" -> "alternative routing algorithms MAY
> be selected"
>
> - Section 5.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.
>
> -Section 5.3.3:
> --"critical" description:  "Whether this extension must be.." "Whether this
> extension needs to be…"
>
> -Section 5.3.4:  1st para after certificate structure:  "may contain" ->
> "MAY contain"
>
> -Section 5.5: It would be helpful to highlight that foundation, priority
> and type are based on ICE, in particular since ICE is not discussed until
> the subsequent section - e.g., "the foundation production" -> "the ICE
> foundation production".
>
> - Section 5.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.
>
> -Section 5.6.1:  I suggest that the sub-sections in this section be moved
> to an appendix and a reference to such be added.
>
> -Section 5.6.3.1.1, next to the last paragraph:
> "when a link may be failing" -> "when a link might be failing"
>
> - Section 9.1: While this is an overview, it seems to me that some of this
> is describing normative behavior (that I don't see specified else).  Per the
> general comment, I would reword the first sentence as follows:
> OLD:
>    The algorithm described here is a modified version of the Chord
>    algorithm.
> NEW:
>     The algorithm described here, Chord-RELOAD is a modified version
>     of the Chord algorithm.
>
> - Section 10.3, 1st para:  "required" -> "REQUIRED"
>
> - Section 10.3.1, last sentence: "must"  -> "MUST"
>
> - Section 10.4, 1st para, last sentence: "should use" -> "SHOULD use"
>
> - Section 11 - examples:
> --General - there are no detailed messages shown anywhere for the examples.
> There should be at least one, in particular given the comment above about
> the RELOAD URI that is registered, but not mentioned anywhere else in the
> document.
> --first example (see my nit below about the examples not being numbered or
> labeled in any way):  The third sentence says that "It gets routed to the
> admitting peer (AP), yet the flow shows that the message first gets routed
> to the PP and then onto AP. It would be helpful if that were clarified.
> -  Also (general), the text mentions the use of ICE.  It's not clear to me
> exactly where in the flow that happens - it would be good to annotate such
> (i.e., in a similar manner as the TLS is illustrated).
> - next to the last example (text on page 131, diagram on page 132): I'm
> quite confused as the text states that JP has received an update from AP and
> thus now knows APs predecessors, which are also JP's predecessors, so JP
> Attaches to them. However, the diagram does not show JP getting an Update
> from AP and it shows JP doing Updates to PPP and PP rather than Attaches (as
> the text suggests).
>
> - Section 13.10. Overlay link types: These specific string types are not
> used elsewhere in the document.  Suggest that a reference to 5.6 be added
> and these particular strings be included in parenthesis after the items in
> the list.
>
>
> - Section 13.16, Security considerations portion:
> -- The first sentence doesn't make sense. Did you mean?
>    "This media type is typically not used to
>    transport information that needs to be kept confidential,
>    however, there are cases where the integrity of the information is
>    important."
> -- "then the contents of the file need to be…" -> "then the contents of the
> file MUST be …"
>
>
>
> Nits:
> -----
> - General: kind is sometimes "Kind" and sometimes "kind".  The former is
> more readable and highlights it as a reserved word.
>
> - Section 2.
> --General: It would be helpful if the terms were alphabetized.
> --Node: "We use the term "Node" to refer to …" -> "Refers to …"
>
> - Section 3.2:
> -- 2nd para, last sentence:  "refer such" -> "refer to such"
> -- 3rd para, 1st sentence:  "concept" -> "entity"
>
> - Section 4.1.4, 4th paragraph: I'm still not getting the applicability of
> voicemail as a RELOAD Kind (per the reference also in section 1.2.1).  I
> guess you are talking about a voicemail message (in this case) and a server
> or application in section 1.2.1?   Voicemail just does not seem like an
> obvious example to me.
>
> - Section 5.1.1, last bullet:  "In that case,…" ->  "In the latter case,
> …"
>
> - Section 5.1.2:
> -- 2nd para:  "arrange" -> "ensure",  "This may be arranged" -> "This can
> be arranged…"
> -- 3rd para (1st after bullets): It's not clear that A…E is a set of
> ordered nodes in the example. I would suggest to reword as:
> OLD:
>    As an example of the first strategy, if node D
> NEW:
>   Consider an example with nodes A, B, C, D and E, respectively.  If
>   node D….
>
> - Section 5.3.1.1: First two sentences can be reworded more succinctly -
> you're not really providing definitions here, put rather describing the
> the syntax for and providing examples illustrative of the presentation
> language.
>
> OLD:
>    The following definitions are used throughout RELOAD and so are
>    defined here.  They also provide a convenient introduction to how to
>    read the presentation language.
> NEW:
>    This section provides an introduction to the presentation language
>    used throughout RELOAD.
>
> - Section 5.3.3.1: Error_Data_Too_Old is duplicated.
>
> - Section 5.5.1.6: Add references for SCTP and DCCP.
>
> - Section 6.4.1.3:  The reference to "this Version" is superfluous.
> Propose the following change:
> OLD:
>    This version of RELOAD (unlike previous versions) does not have an
>    explicit Remove operation.
> NEW:
>    RELOAD does not have an explicit Remove operation.
>
> - Section 9: General. The first sentence states that the algorithm defined
> here is referred to as "chord-reload", however, subsequent sections don't
> seem to use that term.  It would be more clear if that term was used. And,
> really, it would be more consistent to use the term Chord-RELOAD. However,
> this should also be consistent with the IANA-Registration which has
> CHORD-RELOAD.
>
> - Section 9.7.4.3, 3rd para:
> -- Remove "Note to implementers".  Text already highlights that the text is
> specific to implementations.
> -- "may be found" -> "can be found"
>
> - Section 9.8:  "For this topology plugin" -> "For Chord-RELOAD"
>
> - Section 10.2, 2nd para:  "determines" -> "determining"
>
> - Section 11:
> - General: there are no labels for any of the call flows, so it's not
> always clear what the text is referring to.  In general, I think the text
> precedes the flow.
> - "abbreviation" -> "abbreviations"
>
> - Section 13.16:
> --Interoperability considerations:  "knows" -> "known"
>
> - Section 14: "provied" -> "provided"
>
> - Appendix C: Delete now - it's totally useless as is.
>