Subject: [Gen-art] Gen-art last call review of draft-ietf-storm-mpa-peer-connect-07

From: Elwyn Davies <[email-masked]>
Date: Mon, 26 Sep 2011 21:54:57 +0100

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

Please resolve these comments along with any other Last Call comments 
you may receive.

Document: draft-ietf-storm-mpa-peer-connect-07
Reviewer: Elwyn Davies
Review Date: 26 Sept 2011
IETF LC End Date: 26 Sept 2011
IESG Telechat date: (if known) 6 Oct 2011

Summary:
Nearly ready for the IESG. There are some minor issues that should be
addressed plus a number of nits.  The main problems that I see are the
lack of clarity on what an RTR Frame actually is and the need for IANA
registries for DDP stream session codes and error codes which are now
spread over at least two documents each.

Major issues:

Minor issues:
s1: The terms active side and passive side are not explicitly defined in
RFC 5044 but are used as synonyms for the Initiator and Responder which
are.  This connection and the Initiator/Responder terminology needs to
be reproduced in this doc.

s1.1: Where is the RTR frame defined?  I can't find any explicit
specification of what an RTR frame looks like in either this document or
any of the existing RFC 504x documents.  Is s9.2 supposed to be the
definition?

ss6, 7 and 9:  Not So Private Data: What happens if some other update
decides it needs to do the same trick with putting some not really
private data in the private data field?  Might be worth putting a note
about (say) ordering flag bits and chunks of data. 

s7: Given that DDP Stream Session codes are now defined in multiple docs
(RFC 5043 and this one), an IANA registry is probably needed.

s8: Given that error codes are now defined in two documents, an IANA
registry is probably needed.

s10: Para 1 and para 4 are possibly contradictory.  Presumably it is
legitimate for an enhanced responder to choose to behave like an
unenhanced responder if there is some good reason for this (e.g. the
application doesn't want to do enhanced. As para 4 stands it is
impossible for an enhanced responder to behave as unenhanced even though
the initiator has the choice.

s11: See comments on s7 and s8 above.  New registries may be required.

Nits/editorial comments:
Abstract: Various acronyms need expansion (RDMA, MPA) and 
s/RFC50/RFC 50/g

s1: Should probably reference RFC 5041 which is the key specification
for RDDP if I understand correctly.

s1, para 1: Maybe..
  s/Marker protocol Data Unit PDU/Marker Protocol Data Unit (PDU)/

s1, 4th bullet point:
> If accepting, the passive side ULP submits its acceptance of the
>       connection request.  This local accept operation...
It is unclear whether this text is referring to things that are done in
the Responder or sent in the protocol response (or indeed both).  This
needs to be made easier to parse.

Titles of s1.1 and s1.2: s/from/affecting/

s4.1. near end of next to last para: s/uses ORD strictly to limit/uses
ORD to strictly limit/

s4.3: Need to expand MPI acronym. 

s4.4.3, para 2: s/necessarily generates/necessarily generate/

s5, para 6:
> The local interface SHOULD require the ULP to explicitly configure...
Is this actually a protocol requirement?  Presumably the default is the
situation before this draft is implemented (i.e. no explicit RTR).  In
order for the explicit RTR to be useful, the interface has to provide a
way for ULPs to request it.  So I *think* this should be 
	The local interface MUST (or perhaps 'needs to') provide a way for a
ULP to request the use of explicit RTR messages.

s7: As with Section 6 and as noted in Section 9, the new private data
has to be first in the private data field.

s9: Need to specify how IRD and ORD are encoded (presumably 14 bit
unsigned integer, network bit order).