Thanks again Martin for your review. We are deciding today what to do about this document at the IESG telechat. FWIW, I have some of your issues, some from Elwyn, and one of my own. The main issue that I'm raising from your review is the upper/lower case MUST. That was actually problematic for a particular issue that I found as well.
Also, did I miss a response form the authors/WG to this review? I can not find one in my e-mail archives… Jari On Apr 16, 2013, at 3:41 AM, Martin Thomson <[email protected]> wrote: > 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-nfsv4-rfc3530bis-25 > Reviewer: Martin Thomson > Review Date: 2013-04 > IETF LC End Date: too soon > IESG Telechat date: ditto > > Summary: I have some confidence that this document is ready for > publication. What I have seen is sufficiently well specified that it > could be implemented, mostly. There are large parts of this document > that I simply haven't read. I can't justify spending more time. > (Sorry Jari, I tried.) > > Reviewing this document is especially difficult because the it relies > heavily on context. A lot of this context is only gained by reading on > and stumbling upon that context. Much of this could be addressed > with appropriate forward references. > > It's clear that Section 12 is largely new, or at least significantly > changed from RFC 3530. Section 12 on its own would constitute a > larger than average RFC. > > If accessibility is a goal, the document could have been split and it > probably should have been. However, I suspect that accessibility > isn't that important to those working on this. > > The document talks quite frequently about "before", with reference to > NFSv2 or NFSv3. This is almost always useless to a reader without > context. I don't expect you to do anything about this, but I'd > request that any future revision consider removing this. For one, in > reviewing a -bis draft, I personally think of RFC3530 as the "before" > to which is refered. On the whole, fewer history lessons and more > real context would have made this a lot easier to read. > > General: The document uses lowercase "must" and "may" a lot. It's not > clear whether these are intended to be interpreted as normative > statements or not. To my eyes, most of these are. On the other hand, > there are places where 2119 language is used to effectively specify > implementation details that aren't observable at a protocol level. > If the document weren't so large, I'd suggest that the editors go through > and make sure that every 2119 keyword is in uppercase and then to > check each and every instance. > > Nits: > > 1.5.3 - I realize that it might be premature to explain details when the > draft hasn't yet described how to identify locations (that's a truly > unfortunate choice of word). Maybe a short primer on that could be > included. At this point, I'm relying on assumed knowledge about > filesystems to understand concepts like: hierarchical ACLs, pseudo > filesystems, attributes. > > Even the fact that a filesystem is a tree with terminal nodes that > contain sequences of bytes. Non-terminal nodes (directories) and > terminal nodes (files) are named (hence the UTF-8) and have > attributes. Where it gets fuzzy is when it uses "path name". Is > "path name" an abstraction that encapsulates "directory name" and > "file name"? Is a "file name" the name of the node, or is it the > entire locator including the names of the directory above it (and some > unspecified separator)? > > 1.5.3.1 > [...] The server provides multiple > filesystems by gluing them together with pseudo filesystems. These > pseudo filesystems provide for potential gaps in the path names > between real filesystems. > > I don't know how to interpret this last statement. I'm using a fair > bit of guesswork to infer how this works as it is, but that > understanding does not result in gaps, just pseudo filesystems (with > no actual files). > > > 1.5.3.3 - The description of namespaces is singularly unhelpful in > conveying any information about the mechanism. The benefits of this > snake oil are clearly splendrous. > > s/alternate/alternative/ - I know that alternate is acceptable US > English, but alternate means something else to other English speakers. > > 1.5.5 - NLM could probably use a citation. > > 3.1 - "The registered port 2049 [RFC3232] for the NFS protocol SHOULD > be the default configuration." This is an implementation requirement, > not a protocol one. > > - "this will allow NFS to transit firewalls" is only true if the > firewall is configured to recognize NTP using port numbers and allow > its passage. > > - "To date, all NFSv4 implementations are TCP based." Is this > really an appropriate statement for an RFC? > > - "UDP by itself is not sufficient as a transport for NFSv4, > neither is UDP in combination with some other mechanism (e.g., DCCP > [RFC4340], NORM [RFC5740])." This statement is strange, given that > UDP is specifically prohibited. I understand that something layered > on UDP (SCTP perhaps) might be able to meet the requirements, but this > statement implies that neither congestion control (DCCP), nor > reliability (NORM) is sufficient. The draft should enumerate what > properties it expects to obtain from its transport before making such > statements. This section is really fuzzy on reliability expectations > (and 3.1.1 doesn't really help with that). > > 3.2 - "For NFSv4, the RPCSEC_GSS security flavor MUST be used to > enable the mandatory security mechanism." Which aspect of security is > mandatory? I find this confusing. Earlier statements were pretty > clear: you must implement it, but you don't have to use it. > > 3.3 - s/may/can/ > > - The SHOULD here (which is also a SHOULD in S17) could be > stronger. I can understand why you need to allow for deployments that > don't use TLS (or equivalent integrity protection), but this > > 3.3.3 - "For Kerberos V5, the following form is RECOMMENDED: > nfs/hostname" - Q: how do you ensure interoperability if you don't > mandate a format? Comment: This doesn't provide information on why an > implementation might choose a different form. > > 4.2 - The definitions of "persistent" and "volatile" are not known, > which makes this introduction to filehandle types hard to place in > context. In many respects the discussion on the history of the > protocol is unnecessary at this point. This would flow better if the > entire subsection was replaced with: "There are two[*] types of > filehandle: persistent and volatile. Servers select the type that > they provide clients and signal the choice using the mandatory > "fh_expire_type" attribute (see Section 4.2.3)." The rationale for > having both belongs in the respective sections (4.2.2 and 4.2.3). > > * There aren't two types of filehandle, there are six different > persistence guarantees for filehandles. > > 4.2.1 - "Clients MUST use filehandle comparisons only to improve > performance, not for correct behavior. All clients need to be > prepared for situations in which it cannot be determined whether two > filehandles denote the same object and in such cases, avoid making > invalid assumptions which might cause incorrect behavior." This seems > a little confused. The context establishes the fact that equal > filehandles always refer to the same file, and sometimes two different > filehandles can refer to the same file (though they SHOULDn't). So, > what value does the 'MUST' provide here? (I can't see how this would > be testable at the protocol level.) Given that there is more > discussion in 10.3.4, I'd recommend dropping these two sentences. > > 5.1 - Use of lowercase "must" and "may" are confusing. Consider using > alternatives. > > 5.2 - is that lowercase "must" in the third sentence really a MUST? > > - "A client MAY ask for the set of attributes the server supports > and SHOULD NOT request attributes the server does not support." How > does the client ask for the supported set? > > - "A server should provide attributes whenever they don't have to > "tell lies" to the client." This statement is odd. Even with an > edit: "Rather than manufacturing fake attribute values, a server is > encouraged to avoid sending attributes that it can't support." This > seems closer implementation magic than protocol specification. > > 12.1.2 - LATIN SMALL LETTER E is not U+0xxx > > - This section includes basically the same list twice. The > first instance is on Page 179 (the first two-item list), the second on > the top of 180. > > 12.4.2 - What do you do with servers that are identified by server > names? The last paragraph mentions that IP addresses are just ASCII. > Are server names are subject to the IDNA rules? Do you want to > reference Section 12.6? > > 12.7.3 - none of the last four points uses normative 2119 language, > though "must" and "should" appear. The text looks pretty normative to > me. > > - This looks less predictable than it could be. Is it > reasonable to require that a server use the same normalization every > time it returns a name? (That stability would only be required for a > given filehandle, of course.) > _______________________________________________ > Gen-art mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/gen-art _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
