Document: draft-ietf-nfsv4-minorversion1-26.txt Reviewer: David L. Black Review Date: 27 November 2008 IETF LC End Date: 27 November 2008 IESG Telechat Date: 04 December 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@ietf.org." 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.