Last Call Review of draft-ietf-pcp-description-option-03
review-ietf-pcp-description-option-03-opsdir-lc-korhonen-2014-02-04-00
Request | Review of | draft-ietf-pcp-description-option |
---|---|---|
Requested revision | No specific revision (document currently at 05) | |
Type | Last Call Review | |
Team | Ops Directorate (opsdir) | |
Deadline | 2014-02-04 | |
Requested | 2014-01-23 | |
Authors | Mohamed Boucadair , Reinaldo Penno , Dan Wing | |
I-D last updated | 2014-02-04 | |
Completed reviews |
Genart Last Call review of -03 by Roni Even
(diff)
Genart Telechat review of -04 by Roni Even (diff) Secdir Early review of -02 by Phillip Hallam-Baker (diff) Secdir Telechat review of -04 by Phillip Hallam-Baker (diff) Opsdir Last Call review of -03 by Jouni Korhonen (diff) |
|
Assignment | Reviewer | Jouni Korhonen |
State | Completed | |
Request | Last Call review on draft-ietf-pcp-description-option by Ops Directorate Assigned | |
Reviewed revision | 03 (document currently at 05) | |
Result | Ready | |
Completed | 2014-02-04 |
review-ietf-pcp-description-option-03-opsdir-lc-korhonen-2014-02-04-00
I have reviewed the document "PCP Description Option" (draft-ietf-pcp-description-option-03) as part of the Operational directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the operational area directors. Document editors and WG chairs should treat these comments just like any other last call comments. IETF State: Submitted to IESG for Publication IESG State: In Last Call (ends 2014-02-04) IANA Review State: IANA - Review Needed IANA Action State: None The draft is: Almost ready for publication. Summary: The document defines a new DESCRIPTION option that can be used to add a human readable text/note to PCP-instatiated mappings. The option and its semantics are rather straight forward. Below are few comments that are not really pointing out errors just trying to improve the document. Comments: ** Section 2 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | DESCRIPTION | Reserved | Length | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ Small nit but reflecting the RFC6887 and the text in other parts of the document that reference Figure 1 I would change the figure to 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ |Option Code=TBA| Reserved | Length | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ Also Number: <to be assigned in the optional-to-process range> to Number: <TBA> ** Section 3 If the PCP client does not receive a DESCRIPTION option in a response to a request enclosing a DESCRIPTION option, this means the PCP server does not support that Option. If the DESCRIPTION option is This is not entirely align with earlier text. If the response does not contain the DESCRIPTION option it may also mean the server was configured to ignore it.. while the server still understands the option. I would clarify this here, since there is subtle difference between server not understanding or purposely discarding the option. s/Option/option configured maximum length MUST NOT exceed 1016 octets. The suggested maximum length is 128 octets. If a PCP client includes a DESCRIPTION Why not saying the "suggested maximum length" is the "default maximum length"? Would sound a bit more formal. Why there is no similar recommendation for the PCP client? Although I kind of can figure out where the maximum length of 1016 octets come from, it would be nice to have a pointer/reference why 1016 and not 1096? If the description text carried in the DESCRIPTION option is null terminated, the behavior of the PCP server is implementation- specific. The PCP server may decide to discard the option or store Hmm.. earlier it was said that the text MUST NOT be null terminated. Why it needs then the above comment? If the text is null terminated, it is an error in implementation, right? Would be better to send an error or just discard the description in this case instead of trying to add a possibility to do something implementation specific. I recommend removing any text hinting an alternative handling for null terminated text since it is "MUST NOT". Rather use the same treatment as in invalid UTF-8 case. and return the exact description text, including Null characters. s/Null/null ** Section 5 In order to make IANA considerations more clear I would suggest changing DESCRIPTION to DESCRIPTION set to TBA since the body text uses "TBA" in places. IDnits: none to worry about. - Jouni