Telechat Review of draft-ietf-nfsv4-rfc3530bis-33
review-ietf-nfsv4-rfc3530bis-33-genart-telechat-davies-2013-09-11-00

Request Review of draft-ietf-nfsv4-rfc3530bis
Requested rev. no specific revision (document currently at 35)
Type Telechat Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2013-05-28
Requested 2013-05-30
Draft last updated 2013-09-11
Completed reviews Genart Last Call review of -25 by Martin Thomson (diff)
Genart Telechat review of -26 by Elwyn Davies (diff)
Genart Telechat review of -26 by Elwyn Davies (diff)
Genart Telechat review of -33 by Elwyn Davies (diff)
Genart Last Call review of -34 by Elwyn Davies (diff)
Secdir Last Call review of -25 by Yoav Nir (diff)
Assignment Reviewer Elwyn Davies
State Completed
Review review-ietf-nfsv4-rfc3530bis-33-genart-telechat-davies-2013-09-11
Reviewed rev. 33 (document currently at 35)
Review result Almost Ready
Review completed: 2013-09-11

Review
review-ietf-nfsv4-rfc3530bis-33-genart-telechat-davies-2013-09-11

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.