Last Call Review of draft-ietf-pcp-upnp-igd-interworking-07
review-ietf-pcp-upnp-igd-interworking-07-genart-lc-yee-2013-04-09-00

Request Review of draft-ietf-pcp-upnp-igd-interworking
Requested rev. no specific revision (document currently at 10)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2013-04-08
Requested 2013-03-28
Draft last updated 2013-04-09
Completed reviews Genart Last Call review of -07 by Peter Yee (diff)
Genart Telechat review of -08 by Peter Yee (diff)
Secdir Last Call review of -07 by Vincent Roca (diff)
Assignment Reviewer Peter Yee
State Completed
Review review-ietf-pcp-upnp-igd-interworking-07-genart-lc-yee-2013-04-09
Reviewed rev. 07 (document currently at 10)
Review result Ready with Issues
Review completed: 2013-04-09

Review
review-ietf-pcp-upnp-igd-interworking-07-genart-lc-yee-2013-04-09

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-pcp-upnp-igd-interworking-07
Reviewer: Peter Yee
Review Date: Apr-08-2013
IETF LC End Date: Apr-08-2013
IESG Telechat date: Unknown

Summary: This draft is on the right track but has open issues, described in
the review. [Ready with issues]

This is a well-written draft describing how Universal Plug-and-Play Internet
Gateway Devices interwork with NAT devices that use the Port Control
Protocol.  My review is mostly nits with otherwise minor issues.  I have no
problems with the general concept and enjoyed reading a clearly articulated
concept.

Major Issues:

None.

Minor Issues:

Section 4.1: is the mapping of the state variables between the UPnP IGD
function and the PCP Client function really straightforward such that it
does not need further description?  I'm asking if an implementor would
naturally gravitate to the correct use of the mapping without further
instructions.  Similar questions arise for Sections 4.2 and 4.3, although I
believe those are more straightforward mappings.

Page 13, 1st paragraph, 3rd sentence: what's meant here is if any PCP error
other than a short-lifetime error, or in the case of a failed resend, any
PCP error at all.  The wording makes it seem like the short-lifetime errors
are somehow not PCP errors and is therefore confusing.  It also doesn't
explicitly deal with how many repeats should be done on a resend.  

Nits:

General: replace "re-send" with "resend".

Page 3, 1st paragraph, 2nd sentence: insert "a" before each occurrence of
UPnP.  

Page 4, section 3, 4th bullet: consider changing "in" to "on" or "over".

Page 4, 1st paragraph after the bullet items: bracket "for instance" with
commas.

Page 5, Figure 3: perhaps the "LAN Side" label could move a bit to the left
to give it better delineation from the "External Side" label?

Page 5, 1st paragraph after Figure 4, 2nd sentence: add a comma after "
[I-D.ietf-pcp-base]".

Page 5, 2nd paragraph after Figure 4: change "can not" to "cannot".

Page 9, section 5, 3rd paragraph: insert "the same way" or "the same" before
"as any PCP Client".

Page 9, section 5.1, 2nd paragraph: insert "whether" before "it should".

Page 9, section 5.1, 3rd paragraph: insert "the" before "requesting UPnP
Control Point".

Page 10, Section 5.3, 2nd paragraph, 1st sentence: consider changing
"retrieve" to "extract".

Page 11, 1st paragraph after bullet items, 2nd sentence: change "to" to
"than".

Page 11, Section 5.6: swap the order of "AddPortMapping()" and
"AddAnyPortMapping()" to match the order of the subsequent subsections.

Page 11, Section 5.6.1, 2nd paragraph, 2nd sentence: change "30s" to "30
seconds".

Page 13, 1st paragraph, 4th sentence: change "re-issue" to "issue"; change
"new requested" to "different requested".

Page 14, last paragraph: delete the comma after "4 times)".

Page 15, Section 5.7, 1st paragraph: append a comma after
"GetSpecificPortMappingEntry()".

Page 15, Section 5.7, 3rd paragraph, 3rd sentence: replace "60s" with "60
seconds".

Page 15, Section 5.7, 3rd paragraph, last sentence: insert "the" before
"error code"; change "enquried" to "queried".

Page 15, Section 5.8, 1st paragraph: insert ", respectively" before the
period.

Page 15, Section 5.8, 2nd paragraph, 2nd sentence: insert "the" before '"606
Action Not'.

Page 16, last paragraph, 2nd sentence: insert "the" before'"731
PortMappingNotFound'.

Page 19, 1st continuation paragraph, 1st sentence fragment: change "of" to
"in".

Page 19, 1st continuation paragraph, 1st full sentence: consider swapping
the positions of "renew" and "periodically" for readability.

Page 19, Section 5.10, 1st paragraph, 2nd sentence: I prefer "In particular"
to "Particularly" here.

Page 19, Section 5.10, 3rd paragraph, 3rd sentence: replace "signalled" with
"signaled".