Subject: Gen-ART review of draft-ietf-pcp-upnp-igd-interworking-07

From: "Peter Yee" <[email-masked]>
Date: Tue, 9 Apr 2013 01:12:49 -0700

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

---

From: <[email-masked]>
Date: Tue, 9 Apr 2013 14:26:55 +0200

Dear Peter,

Thanks for the review. 
Please see inline.

Cheers,
Med

>-----Message d'origine-----
>De : Peter Yee [mailto:[email-masked]]
>Envoyé : mardi 9 avril 2013 10:13
>À : [email-masked]
>Cc : [email-masked]; [email-masked]
>Objet : Gen-ART review of draft-ietf-pcp-upnp-igd-interworking-07
>

[Med] Thanks.

>believe those are more straightforward mappings.

[Med] I don't think further explanation is needed. Pointers to the appropriate sections is included in Section 4.2 and Section 4.3. FWIW, there are already existing implementations of the IWF and no "complaint" was received from these implementers.

>explicitly deal with how many repeats should be done on a resend.

[Med] The basic behavior is to relay the received error to the UPnP CP. For the short-lifetime errors, the IWF may decide to resend the request and not relay those errors immediately to the UPnP CP. The number of repeats is not specified here as it can be implementation-specific. 

>Nits:

[Med] Fixed.

---

From: "Peter Yee" <[email-masked]>
Date: Tue, 9 Apr 2013 11:57:50 -0700

Med,
	Thanks for the swift response to my review.  See my one reply
inline.

		Kind regards,
		-Peter

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

>[Med] The basic behavior is to relay the received error to the UPnP CP. For
the short-lifetime errors, the IWF may decide to resend the request and not
relay those errors immediately to the UPnP CP. The number of repeats is not
specified here as it can be implementation-specific. 

Your explanation is fine.  I just found the wording "If a PCP  error
response is received" to sound ambiguously as if the short-lifetime errors
were not a subset of PCP errors.

---

From: <[email-masked]>
Date: Wed, 10 Apr 2013 09:49:25 +0200

Dear Peter,

I changed the text as follows:

OLD:

   If the requested external port is not available, the PCP server will
   send a CANNOT_PROVIDE_EXTERNAL error response.  If a short lifetime
   error is returned, the IGD-PCP IWF MAY re-send the same request to
   the PCP Server after 30 seconds.  If a PCP error response is
   received, the IGD-PCP IWF relays a negative message to the UPnP
   Control Point with ConflictInMappingEntry as the error code.

NEW:

   If the requested external port is not available, the PCP server will
   send a CANNOT_PROVIDE_EXTERNAL error response:

   1.  If a short lifetime error is returned, the IGD-PCP IWF MAY resend
       the same request to the PCP Server after 30 seconds without
       relaying the error to the UPnP Control Point.  The IGD-PCP IWF
       MAY repeat this process until a positive answer is received or
       some maximum retry limit is reached.  When the maximum retry
       limit is reached, the IGD-PCP IWF relays a negative message to
       the UPnP Control Point with ConflictInMappingEntry as the error
       code.

   2.  If a long lifetime error is returned, the IGD-PCP IWF relays a
       negative message to the UPnP Control Point with
       ConflictInMappingEntry as the error code.

Better?

Cheers,
Med

>Envoyé : mardi 9 avril 2013 20:58
>À : BOUCADAIR Mohamed OLNC/OLN; draft-ietf-pcp-upnp-igd-
>[email-masked]
>Objet : RE: Gen-ART review of draft-ietf-pcp-upnp-igd-interworking-07
>
>Med,

---

From: "Peter Yee" <[email-masked]>
Date: Wed, 10 Apr 2013 09:46:55 -0700

Med,

	That looks great.  Thanks for accommodating my concern.

			Kind regards,
			-Peter

-----Original Message-----
From: [email-masked] [mailto:[email-masked]] 
Sent: Wednesday, April 10, 2013 12:49 AM
To: Peter Yee; [email-masked]
Cc: [email-masked]; [email-masked]
Subject: RE: Gen-ART review of draft-ietf-pcp-upnp-igd-interworking-07

Dear Peter,

OLD:

NEW:

Better?

Cheers,
Med

>De : Peter Yee [mailto:[email-masked]] Envoyé : mardi 9 avril 2013 
>20:58 À : BOUCADAIR Mohamed OLNC/OLN; draft-ietf-pcp-upnp-igd- 
>[email-masked] Cc : [email-masked]; [email-masked] 
inline.
>the short-lifetime errors, the IWF may decide to resend the request and 
>not relay those errors immediately to the UPnP CP. The number of 
>repeats is not specified here as it can be implementation-specific.
>
>response is received" to sound ambiguously as if the short-lifetime 
>errors were not a subset of PCP errors.
>

---

From: <[email-masked]>
Date: Thu, 11 Apr 2013 07:43:53 +0200

Dear Peter,

The new version is now available online. A diff is available here: http://www.ietf.org/rfcdiff?url2=draft-ietf-pcp-upnp-igd-interworking-08.

Thank you again for the review.

Cheers,
Med

>Envoyé : mercredi 10 avril 2013 18:47