Hi Elwyn, Thanks for the comprehensive review (again).
I’m probably 2/3rds done with the easy changes and I expect to reply within the next two days. Tom > On Nov 11, 2014, at 12:48 PM, Elwyn Davies <[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-33.txt > Reviewer: Elwyn Davies > Review Date: 2014-11-06 > IETF LC End Date: 2014-10-06 > IESG Telechat date: 2014-12-04 > > Summary: > Much more ready than last time I reviewed this draft! There are a few issues > at the borderline between editorial and minor plus quite a lot of editorial > nits. > > Major Issues: > > Minor Issues: > s8.8.1: I suspect that this section should have been deleted when s7.7 was > deleted between v26 and v27. The references to various server class > attributes (handle class, change class, etc) are orphaned with no definitions > of these items > > ----------------------------------------- > > Handling of wrap around of seqid4 variables: > ============================================ > The specification of how variables/fields of type seqid4 are incremented and > compared, dealing with wrap around is very messy. > Currently there are two separate descriptions of handling wrap around when a > seqid4 variable is incremented through NFS4_UINT32_MAX (which turns out to be > the all ones binary pattern. The descriptions are slightly different but not > incompatible. > The following text appears in s9.1.6 >> Since the sequence number is represented with an unsigned 32-bit >> integer, the arithmetic involved with the sequence number is mod >> 2^32. Note that when the seqid wraps, it SHOULD bypass zero and use >> one as the next seqid value. For an example of modulo arithmetic >> involving sequence numbers see [RFC0793]. > and s9.1.3.2 has: >> This pattern continues until the seqid is incremented past >> NFS4_UINT32_MAX, and one (not zero) SHOULD be the next seqid value. >> The purpose of the incrementing of the seqid is to allow the server >> to communicate to the client the order in which operations that >> modified locking state associated with a stateid have been processed. >> >> In making comparisons between seqids, both by the client in >> determining the order of operations and by the server in determining >> whether the NFS4ERR_OLD_STATEID is to be returned, the possibility of >> the seqid being swapped around past the NFS4_UINT32_MAX value needs >> to be taken into account. >> > The definition of the seqid4 type in s2.1 doesn't mention this sort of > constraint. > > Incrementing of seqid4 type fields is mentioned in a number of sections along > with special values (including 0): > s9.1.3.1, bullet 1 (also bullet 2): >> Such stateids are subject >> to change (with consequent incrementing of the stateid's seqid) in >> response to OPENs that result in upgrade and OPEN_DOWNGRADE >> operations. > > s9.1.3.3: Refers to special values of seqid (all zeroes, all ones) > > s9.1.3.4, two bullets before last bullet have: >> o If the "seqid" field is not zero, and it is greater than the >> current sequence value corresponding the current "other" field, >> return NFS4ERR_BAD_STATEID.draft-ietf-nfsv4-rfc3530bis >> >> o If the "seqid" field is less than the current sequence value >> corresponding the current "other" field, return >> NFS4ERR_OLD_STATEID. >> > > s9.11, last para: >> The "seqid" value in the returned stateid >> MUST be incremented, even in situations in which there is no change >> to the access and deny bits for the file. > > s15.8.5, para 3 before last: >> In this case, the >> stateid returned as an "other" field that matches that of the >> previous open while the "seqid" field is incremented to reflect the >> change status due to the new open. >> > It would be much cleaner if the texts from s9.1.3.2 and s9.1.6 were merged, > probably into a section just after 2.1 (say s2.1.1) together > draft-ietf-nfsv4-rfc3530biswith mention of the special values. If this is > done, s2.1.1 can then be referenced wherever a seqid is mentioned as being > incremented. > > Also, I cannot see why the current texts use SHOULD. I am pretty certain > that skipping 0 and going to 1 from NFS4_UINT32_MAX is a MUST. If it isn't a > MUST, what alternative scheme will make the wrap around work without both > client and server knowing what scheme is being used? This is at least partly > implied by the definition of NFS4ERR_BAD_SEQID in s13.1.8.2 which indicates > that both ends have a common idea of what the next expected number ought to > be.. > > ----------------------------------- > > change_info4: Wrap around in before and after fields. > ====================================================== > I assume that it is deemed that using a 64 bit number as changeid4 should > obviate the need for dealing with wrap around. However I don't think there > is any advice on starting values for the change values - some strategies > (e.g., random values) could result in change indicators starting > uncomfortably close to the maximum value and running up against wrap around. > A brief note on this would be desirable. > > ----------------------------------- > > s10.7: Informing the server that a file is being memory mapped. > Is there a mechanism? If not should there be (e.g., a flag on OPEN)? > > ----------------------------------- > > s15.2.3, paras 4 and 7: The last sentence in para 7 that implies > NFS4ERR_MINOR_VERSION_MISMATCH takes priority over all others appears to be > incompatible with the second XDR decoding strategy in para 4. If decoding of > the operation array is interleaved with performing the requested operations > then it seems possible that the COMPOUND may be terminated by an earlier > error before the offending operation 2 is reached, so that it would never be > decoded. Further, I don't understand why operation 2 is singled out for > special discussion here, other than maybe to say that it is available for > future use. How does it differ from any other value that is not supported by > the minorversion implemented by the server? Why should the base version be > singled out for special treatment? > > Nits/editorial comments: > ======================== > > Requirements Language note (after Abstract): The terms REQUIRED and > RECOMMENDED are overloaded in this document, in that they are used both in > the RFC 2119 sense and as adjectives that characterize attributes. It would > be a good idea to avoid confusion by providing an extension of the usual form > of words in this section, such as: > NEW: > The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", > "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this > document are to be interpreted as described in RFC 2119 [RFC2119] > except where "REQUIRED" and "RECOMMENDED" are used as qualifiers > to distinguish classes of attributes as described in Sections > 1.3.5.2 and 5. > > > General: There are a small number of instances where the form '<name> > recommended attribute' and similar is used. It would be clearer to use > 'RECOMMENDED attribute in these cases: > - s15.17.5, next to last para. > - s15.37.5, next to last para. > - s2.2.7 > > s1.3.3.2: Worth reinforcing that attribute names are (presumably) encoded in > utf8 as they are just fileobject names. > > s1.3.4: A pointer to s9.9 for an explanation 'share semantics' would be very > useful. Copying the first para of s9.9 or something similar to the > terminology section would also help. > > s1.4, Definition of 'Lease', para 2: > OLD: > All leases granted by a server have the same fixed interval. Note > that the fixed interval was chosen to alleviate the expense a > server would have in maintaining state about variable length > leases across server failures. > NEW: > All leases granted by a server have the same fixed duration. Note > that the fixed interval duration was chosen to alleviate the > expense a > server would have in maintaining state relating to variable length > leases across server failures. > > s1,4, Definition of Stateid: Definitions of open-owner and lock-owner would > be helpful. The terms are used a few times before they are defined in s9. > > s2: Useful to add a reminder that the size constants and definitive > definitions can be found in [RFCNFSv4XDR]. > > s2.1: Should this have typedef opaque linktext4<>; instead of > typedef opaque linktext4; ? > > s2.2.5: s/mandatory/REQUIRED/ > > s2.2.10: > OLD: > The r_netid and > r_addr fields respectively contain a netid and uaddr. The netid and > uaddr concepts are defined in [RFC5665]. The netid and uaddr formats > for TCP over IPv4 and TCP over IPv6 are defined in [RFC5665], > NEW: > The r_netid and r_addr fields respectively contain a network id and > universal address. The network id and universal address concepts > together with formats for them with TCP over IPv4 and TCP over IPv6 > are defined in [RFC5665], > > s3.3: The terms 'policy boundary' and 'crossing policy boundaries' are used > in subsequent sections but not explicitly defined. I assume that they refer > to the client accessing fileobjects in areas accessed via the different entry > points referred to in s3.3. Assuming I am right introducing the 'policy > boundary' term in s3.3 would be helpful. > > s3.3.3, para 1: s/the security flavor the original SETCLIENTID/the security > flavor of the original SETCLIENTID/ > > s4.2.3, para 2: s/mandatory/REQUIRED/ > > s5.3: I think it would be worth being explicit as to whether hard links can > be created between named attribute fileobjects either (1) for named > attributes of the same fileobject and/or (2) for two different fileobjects on > the same file system. Is it also the case that if the basic file system > supports hard links (link_support == TRUE) then it will necessarily support > hard links between named attributes? As I understand it, the named attribute > system in NFSv4 was modelled on Solaris v9+ and the extended attribute system > in Linux would probably not support hard links (I haven't tried them (yet)) > even if the Solaris version does.... Later... I see from s13.1.4.14 > (NFS4ERR_XDEV) that hard links between different named attribute directories > are NOT allowed. It would be good to make this explicit here in s5.3. This > still leaves the question of whether hard links are allowed in a single named > attribute directory. I can't see a lot of use for this capability but it > should be explicitly allowed or disallowed as appropriate. > > s5.6: There is no definition of nfs_lease4 in either this draft or the xdr > draft. > > s5.8.2.14: Presumably this is hard links: s/links/hard links/; I assume that > the number of symlinks is essentially unconstrained. > > s5.10, last para: it would be a good idea to add a note to the RFC Editor to > ensure that the statements about the Unicode version will be true on > publication! > > s5.10, last para: Is it conceivable that a client would need to know what > version of Unicode a server was using ? If so should there be a way for the > client to find out what version of Unicode is in use by the server? > > s6.1: >> * Setting only the mode attribute should provide reasonable >> security. For example, setting a mode of 000 should be enough >> to ensure that future opens for read or write by any principal >> fail, regardless of a previously existing or inherited ACL. > I think this may only apply to non-privileged principals - root can still > read/write *ix files with mode 0. Tie in with last para of s6.3.1.1. > > s6.2.1, next to last para: I suppose SMB really ought to have a reference. > > ????? s6.2.1.3, last para: The "optional" and "RECOMMENDED" should be in the > same case - I think this should be both upper case RFC 2119 requirements but > it is arguable they should both be lower. > > s6.4.3: It is (still) not clear (at least, to me) how inheritance is > supposed to apply to named attribute directories. The named attribute > directory has the file object to which it applies as its 'parent' rather than > an ordinary directory. Presumably inheritance would apply from the named > attribute directory to the named attributes created in it - but are ACLs > always supported on named attributes and the named attribute directory if > they are suported on ordinary files and directories? > > s7.8, para 1: I am unable to parse the 3rd sentence in this para: >> However, with the support of multiple security mechanisms >> and the ability to negotiate the appropriate use of these mechanisms, >> the server is unable to properly determine if a client will be able >> to authenticate itself. > > > > s8: s/OPTIONAL/optional/ - using multi-server namespaces is a user choice > not a protocol requirement. > > s8.5: >> If a single location entry designates multiple server IP addresses, >> the client cannot assume that these addresses are multiple paths to >> the same server. In most cases, they will be, but the client MUST >> verify that before acting on that assumption. > Is there some means in NFSv4 to do this? I couldn't see that there was. I > guess one could use Reverse DNS. If an out-of-band verification is needed, > this should be made explicit. > > s8.8: Copying the structure definitions from ss2.2.6/2.2.7 is unnecessary and > could be better handled with refs. > > s8.8.1, first and second bullets: > [This comment is redundant if s8.8.1 gets deleted as suggested in Minor > Issues above.] >> o All listed file system instances should be considered as of the >> same handle class if and only if the current fh_expire_type >> attribute does not include the FH4_VOL_MIGRATION bit. > What if fh_expire_type has FH_VOLATILE_ANY set? This is defined in s4.2.3 to > imply FH4_VOL_MIGRATION making the 'setting of FH4_VOL_MIGRATION redundant'. > The same issue applies to the second bullet. > > s9.1.1, 2nd para after 2nd set if bullets (near the bottom of p98): > s/the client for purpose/the client for the purpose/ > > s9.1.1, last para: forward refs to the sections defining SETCLIENTID and > SETCLIENTID_CONFIRM would be helpful. > > s9.1.3.1:s/of guarantee/of a guarantee/ > > s9.1.5, para 2 and elsewhere in s9; also s10.4.1: >> If no state is established by the client, either >> byte-range lock or share reservation, a stateid of all bits 0 is >> used. > Does this differ from the 'anonymous stateid' defined in s9.1.3.3? If not it > would be better to refer to it by this title. There are a number of other > places in s9 where the expressions like 'stateid of all 0s/1s' are used. It > would be more consistent to use the 'anonymous' and 'READ bypass' titles set > up in s9.1.3.3 in all cases. > > s9.1.9, para 3: >> In any event, the server is able to safely release state-owner state >> (in the sense that retransmitted requests will not be erroneously >> acted upon) when the state-owner no currently being utilized by the > ^^^^^^^^^^^^^^^^^^^^^^^^ >> client (i.e., there are no open files associated with an open-owner >> and no lock stateids associated with a lock-owner). > Maybe 'state-owner is not currently'? > > s9.1.9, para 3: > OLD: >> However, the period it chooses to hold >> this state is implementation specific. > > NEW: >> However, the period for which it chooses to hold >> this state is implementation specific. > > s9.1.10, para 1: > s/the sequence number may have any value./the sequence number may have any > valid (i.e., non-zero) value./ > > s9.1.10, last para: >> thus can ensure that >> confirmation will not be required. > s/ensure/be confident/ > > s9.2 (2 places),s10.5,s13.1.8.8: s/lock owner/lock-owner/ (total of 4 > instances) - might be others at ends of lines that I missed. > > s8.6 (4 places), s9.6.1: s/open owner/open-owner/ (total of 5 instances) > > s9.6.3.1, 2nd bullet: s?IO?I/O? > > s9.6.3.1, 3rd bullet: s/haled as courtesy/haled as a courtesy/ > > s9.8, para 5: >> The client may accomplish this task by >> issuing an I/O request, either a pending I/O or a zero-length read, >> specifying the stateid associated with the lock in question. > How do you issue a "pending I/O request"?? > > s9.14.4, title: s/Lease_time/lease_time/ > > s10.2.1, para 9: >> Note that the requirement stated above is not meant to imply that >> when the client is no longer obliged, as required above, to retain >> delegation information, that it should necessarily dispose of it. >> Some specific cases are: > I think this para is referring to the server rather than the client: > s/client/server/. > > s10.2.1, para 26 (half way down p141): >> Whereas, for locks and share reservations, freeing of >> locks will occur immediately upon the appearance of a conflicting >> request, for delegations, the server may institute period during >> which conflicting requests are held off. > s/server may institute period/server MAY institute a period/ > > s10.3.1, 1st bullet: s/The implementer is cautioned in this approach./The > implementer is cautioned against this approach./ > > s10.4.3, next to last para: s/break down/breaks down/ > > s10.7, para 2: s/access remotely/accessed remotely/ > > s11: For the avoidance of confusion, it would be wise to note that REQUIRED > and RECOMMENDED are referring to the RFC 2119 usage rather than the > description of attributes in this section. > > s11, item 2; s15.4, next to last para: The implication of the illegal > operation test in s15.4 is that there should be no gaps in the allocated > operation number sequence. The operation numbers for minor version X must > start immediately after those for version (X-1) and be in a continuous group. > > s11, item 11; s15.4: If for some arcane reason a server supports minor > version X (X >= 2) but does not support minor version Y (0 < Y < X) as > allowed by the SHOULD in s11, should the server return NFS4ERR_NOTSUPP or an > XDR decode error. I would say that the SHOULD could lead to some > considerable confusion for clients without a way to find out what versions a > server implements. > > s11, item 12: Some clarification of the term 'infrastructural' is needed. > > s12.4: Has the dreaded BOM ever been encountered in the UTF-8 strings created > by NFSv5 servers or clients? > Therefore, the mask returned enumerating which access rights can be > determined will have the ACCESS4_DELETE value set to 0. > > s12.4, 1st bullet: s/non-UT8-name/non-UTF-8 encoded name/ > > s12.5, last para:@ s/without modifying name/without modifying the name/ or > maybe "the names". > > s12.6: The reference to RFC 5890 needs to be one para earlier to go with the > first occurrence of U-label and should be added for A-label. > > s13.1.10.1: s/client id/clientid/ > > s15.2.5: >> The client SHOULD NOT construct a COMPOUND which mixes operations for >> different client IDs. > Is this advisory on the grounds that it makes recovery difficult - in which > case s/SHOULD NOT/should not/ - or something that might result in the server > throwing an error - in which case please point to some discussion of why this > is not legal. Later... this is also considered in s15.18.6, last para. So > there is one set of circumstances when it will screw up - but there are > others where it is not problematical. Perhaps this can be explained more > clearly. > > > > s15.3.5: >> Therefore, the mask returned enumerating which access rights can be >> determined will have the ACCESS4_DELETE value set to 0. > s/determined/supported/ > > s15.11: Should this section talk about maxlink and the return of > NFS4ERR_MLINK? > > s15.16.4: LOOKUPP is said to return the parent directory of a named attribute > directory. Arguably a named attribute directory doesn't have a parent... so > does LOOKUPP return the directory in which the object to 'owning' the named > attribute directory resides? Or does it return the owning object? Please > clarify in the text. > > s15.18.2/s15.18.6/s15.21.2/s15.21.4, share_access and share_deny arguments: > Presumably the shorthand DENY/READ/WRITE/BOTH values referred to are the > relevant constants defined in s9.9? If so please add a reference (and maybe > use the full names in s15). Otherwise please define the values. > > s15.18.6, para 9: >> If the component provided to OPEN resolves to something other than a >> regular file > Worth pointing out this includes named attribute files. > > s15.23.4: > OLD: > The public filehandle represents the concepts embodied in [RFC2054], > [RFC2055], [RFC2224]. The intent for NFSv4 is that the public > filehandle (represented by the PUTPUBFH operation) be used as a > method of providing WebNFS server compatibility with NFSv2 and NFSv3. > NEW: > The public filehandle concept was introduced with WebNFS described > in [RFC2054], [RFC2055], and [RFC2224]. The intent for NFSv4 is > that the public filehandle (provided by the PUTPUBFH operation) be > used as a method of providing compatibility with the WebNFS server > extension of NFSv2 and NFSv3. > > s15.23.5, last para: s/public filehandle a method/public filehandle as a > method/ > > s15.26.4/s15.26.5: What happens if the directory has been modified (entries > added or deleted) between calls of READDIR intended to retrieve segments of > the directory information? Related to this, presumably no ordering of > entries is implied in the returned information - it would be desirable to > state this. > > s15.29.4: >> If the oldname refers to a named attribute and the saved and current >> filehandles refer to different file system objects, the server will >> return NFS4ERR_XDEV just as if the saved and current filehandles >> represented directories on different file systems. > Presumably "filehandles refer to different file system objects" means the > filehandles refer to the named attribute directories of different file system > objects. If so then add "the named attribute directories of". > > s15.30.5, 1st bullet: s/client id/client ID/ > > s15.35.5, 1st bullet: s/client id/client ID/ > > s17, para 6: s/the client migrate its traffic/the client to migrate its > traffic/ > > s17, para 7: s/is checked against and match/is checked against and matches > with/ > > s17, para 8: s/see 5.9 and 12/see Sections 5.9 and 12/ > > s18.1, para 4: I don't believe you can have both policies of FCFS and > Specification Required at the same time, and then with Expert Review for > updates. The proposal sounds like FCFS with a requirement that a description > of format and intended usage is provided. > > s19.2/s5.6: I guess it was intended... s/ISEG_errata/IESG_ERRATA/ > > s19: RFC 2279 (UTF-8) is referenced as normative with RFC 3629 which > obsoletes RFC 2279 as informative. I think that references to RFC 2279 > should be replaced by RFC 3629 and this one made normative. > > s19.2: Arguably RFC 5226 (IANA considerations) is normative. _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
