This is a combined Gen-ART and Transport Area review, hence the introductory "boilerplate" for both reviews follows: I have been selected as the General Area Review Team (Gen-ART) reviewer for this draft (for background on Gen-ART, please see http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html). I've reviewed this document as part of the transport area directorate's ongoing effort to review key IETF documents. These comments were written primarily for the transport area directors, but are copied to the document's authors for their information and to allow them to address any issues raised. The authors should consider this review together with any other last-call comments they receive. Please always CC tsv-dir@xxxxxxxx if you reply to or forward this review. Please resolve these comments along with any other Last Call comments you may receive. Document: draft-ietf-nfsv4-minorversion1-26.txt Reviewer: David L. Black Review Date: 27 November 2008 IETF LC End Date: 27 November 2008 Summary: This draft is basically ready for publication, but has nits that should be fixed before publication. Comments: In general, this is a well written draft and reasonably readable despite its length. The draft has had extensive review in the WG and there are multiple known interoperable implementations of much of its contents. As a result, this review focuses on clear exposition of requirements. I found one technical issue that must be addressed: Section 5.10 on character case for Unicode references a very old RFC (RFC 1345) and provides a case determination algorithm (substring match on "CAPITAL" or "SMALL" in a Unicode character name) that is wrong and potentially dangerously so. This section needs to be rewritten to reference a Unicode document that defines the notion of case for a particular version of Unicode. The good news is that everything needed to get this right is in Section 14's profiles of "stringprep" (Unicode 3.2 is referenced from Section 14 via RFC 3454), so a small rewrite of Section 5.10 to remove the case determination algorithm and direct the reader to Section 14 for all internationalization details will probably fix this. I would remove the reference to RFC 1345 as part of this. Minor Technical Items: The draft is a bit cavalier about use of lower case "must" and "should". I found a number of these that arguably should be upper case, but nothing that struck me as crucial to change. Section 2.9.1 - at the end of the first sentence in the last paragraph, please add ", as a consequence UDP by itself MUST NOT be used as an NFSv4.1 transport". This is clearly a consequence of the stated requirements, but one that is worth calling out explicitly. Section 2.10.1 - I was looking for at least a "MUST support" and possibly a "MUST use" requirement for sessions in this section and didn't see either requirement stated. At least "MUST support" needs to be added here. Section 2.10.8.1 - "The connection cannot be hijacked by an attacker who connects to the client port prior to the intended server as is possible with NFSv4.0." I think that sentence is too strong, and that what was meant is that NFSv4.1 provides security measures (e.g., authentication) that enable this form of hijacking to be prevented. Section 2.10.9, 3rd paragraph - "the input text as represented by the XDR encoded enumeration of type ssv_subkey4." That's a bit too terse. Changing "enumeration" --> "enumeration value for that subkey" would be clearer. p.80 after item 4. - "If there is a reconfiguration event which results in the same network being assigned to servers where the eir_server_scope value is different," Should that be "same network address" rather than "same network" ? p.85 - The description of "length4" is "Describes LOCK lengths." The length4 type is used for a lot more than locks, so the description needs to be generalized. p.91, Section 3.3.15 - Change "will be defined" to "are defined" in the last sentence. p.103, Section 5.4 - Explain what the implementation implications are of the attribute class (per server vs. per FS vs. per FS object). Section 5.8.* and elsewhere - consider including the data type of the attribute in the name of the section that defines the attribute or in the text of that section. p 114, Section 5.8.2.30 - The last paragraph presumably refers to selection of the quota set that provides the contents of the quota_used attribute. That should be stated explicitly. p.117, middle of page - "The "dns_domain" portion of the owner string is meant to be a DNS domain name. For example, user@xxxxxxxxx" Change "ietf.org" to "example.org". See ID-Checklist item 3.6 and RFC 2606. p.118, end of Section 5.9 - Add a "MUST NOT" requirement forbidding use of the "nobody" string to represent an actual owner principal. p.135, ACE4_SYNCHRONIZE - Explain what "synchronized reads and writes" are synchronized with/to. p.141, Section 6.3.1.1 - Add a bullet item indicating that retention may also block access otherwise allowed by ACLs with a pointer to Section 5.13. p.196, near top of page - "The implementor is cautioned in this approach." This needs be rewritten as a requirement that clients SHOULD NOT or MUST NOT do what has been described, because doing so may cause data corruption. Editorial Nits: p.14 - The definition of "Stable Storage" doesn't actually define the term. Rephrase start of definition to "Storage from which data stored by an NFSv4.1 server can be recovered without data loss after ..." plus change "and hardware failures" to "and/or hardware failures". p.15 - Section 1.6, second sentence, change "distinguish those added" to "distinguish those features added". p.15 - Section 1.6.1, this should be in present tense, so change first two instances of "will be used" to "is used" and the third one to "are used". Section 2.7 - the minor versioning rules contain multiple instances of "may not". These should all be changed to "must not". p.241 - " The consequences of having no facilities available to reclaim locks on the sew server will depend on the type of environment." sew --> new . I found a number of phrasing nits that I will leave to the RFC Editor to deal with. idnits 2.10.02 flagged a lot of things that don't need attention, but found a few that do: - "RFC 2119" is missing from reference [1]. - The boilerplate should be updated if the draft is revised. - Reference [19] could use some more elaboration. Would a URL be appropriate? - There's now a -10 version of the pnfs block layout draft. Thanks, --David ---------------------------------------------------- David L. Black, Distinguished Engineer EMC Corporation, 176 South St., Hopkinton, MA 01748 +1 (508) 293-7953 FAX: +1 (508) 293-7786 black_david@xxxxxxx Mobile: +1 (978) 394-7754 ---------------------------------------------------- _______________________________________________ Ietf@xxxxxxxx https://www.ietf.org/mailman/listinfo/ietf