Subject: [Gen-art] Gen-art additional review of draft-ietf-nfsv4-rfc3530bis-25

From: Elwyn Davies <[email-masked]>
Date: Wed, 08 May 2013 23:35:21 +0100

I am another 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: Elwyn Davies
Review Date: 8 May 2013
IETF LC End Date: -
IESG Telechat date: 30 May 2013

Summary:
This is a mammoth document.  The comments run to 650 lines.  Clearly 
there ae issues in modifying a -bis document given that it has to be 
pretty much compatible with its progenitor  - but that is now going on 
14 years old. I believe that there are a significant number of minor 
issues to address and parts of the document (despite the overall size) 
need to provide a bit more explanation of what is going on.  In 
particular I think that a little bit more rationale over the way in 
which an NFS server attempts to provide something like a common 
interface to the two big player filesystem types (*ix and Windows) 
[Aside: I am disappointed that it doesn't try to handle versioning file 
systems like the venerable VAX filing system! ;-) ] There are some areas 
where the addition of named attributes hasn't really been properly dealt 
with AFAICS. One major area that I felt ought to have been better 
addressed in the -bis was internationalisation.  As mentioned below, I 
don't understand why additional atrributes couldn't be added to help 
clients understand what the server is doing with internationalisation - 
the client has to do some complex acrobatics which seem close to 
guesswork to work out what is going on in the server.   As ever, I am 
not an expert in NFSv4 (although I used v2 extensively almost 30 years 
ago... arrrgghhh!) so there may well be comments where I don't 
understand the problem.

Major issues:

Overall section comments:
s6: AccessControl Attributes
Combining two different access control paradigms (UNIX-clone mode 
control bitmasks and access control lists) is an uphill struggle and 
provides a deal of complexity in situations where both, either one or 
neither may be specified.  The detailed text deals reasonably with the 
various cases.  There are, in my view, a couple of significant problems:
- The first is essentially editorial:  I would say that rather more than 
basic understanding of UNIX-like file systems is needed.  An explanation 
or at least a suitable reference to explain the use of the mode bits in 
s6.2.2, especially the first three (SUID, SGID, SVTX) would help, and a 
clearer linkage to the explanations of owner, group and other sets of 
users that are distributed in parts of the text.
- The second is more fundamental:  The intended functionality of 
inheritance is not explicitly documented.  To come to any sort of 
understanding of how inheritance is supposed to work you have to read 
through to the next to the last section (s6.4.3).  I have now read 
through the section two or three times and I have a model in my mind, 
but there is no text that would allow me to verify my understanding.  I 
*think* i have understood that (1) inheritance only applies when you 
initially create an object rather than being fed through to all 
'inferior' objects in the tree if a superior object has a heritable ACE 
added; (2) inheritance only applies if you don't explicitly give any 
ACEs when the object is created, and (3) if you give a mode but no ACL 
then some complex combination of inherited ACL and mode is applied.  An 
upfront explanation of what is trying to be achieved would have helped a 
lot.  However, none of this explains what happens with either hard or 
symbolic links.  It is unclear to me whether inheritance of ACLs is 
applied when creating a hard or symbolic link to an existing object.

s7, Multi-Server Namespace:
This section is very verbose and contains a number of areas that bring 
in topics that  are actually forward references without indicating where 
the information can be found, particularly in relation to various 
'change classes'.  Some reordering might help. The whole concept of 
change classes seems a little odd, because when they are finally defined 
(s7.9.1), it appears that servers are rarely in the same class during 
any of the interesting transitions (s7.9.1 says that in most cases they 
have to be taken as in different classes).  Overall, this section 
contains an awful lot of discursive specification relating to servers 
that may not have any coordination which makes it very difficult both 
from the specification and the implementation angles to check that 
everything is working.

s8, NFS Server namespace:  This is a important section for overall 
understanding and I think it would help to have it much earlier, 
certainly before s7.

s9, File Locking and Share Reservations:
Generally very well written, but the treatment of share reservations is 
very skimpy,
It would also be good to be more 'honest' about the interaction with the 
semantics of the underlying file system as regards locks as described in 
s15.12.

s10, Clientside Caching:
Clearly very complex to implement but seems to be well-described.  A bit 
of earlier explanation of the use of CLAIM_PREVIOUS vs 
CLAIM_DELEGATION_PREV in client/server reboots would help.

s11, Minor Versions:
No particular issues.

s12, Internationalisation:
A minefield.  There seems to be an awful lot of guess work for clients 
trying to decide how the sever behaves.  On reflection, I am surprised 
that several of the things that clients are supposed to deduce (e.g., 
what normalisation the server does) were not supplied as attributes of 
the server.  Is there some good reason for this?  I was also surprised 
that there didn't seem to be a requirement that servers working on a 
particular filesystem all did the same internationalisation things.  It 
appears to me that if a filesystem gets migrated the client doesn't have 
any sort of guarantee that the internationalisation behaviour won't 
change under its feet - and it has no easy way to check if this is or is 
not the case because of the lack of attributes.  I have no idea to what 
extent the internationalisation is implemented in current servers.

s13, Error Codes:
Generally OK but with a few code value inconsistencies.  I haven't 
checked the cross-consistency of Table 9 and Table 11 or whether the 
sets of error codes per operation are really right - they are just too 
voluminous.

s14, NFSv4 Requests:
No problems.

s15/16, Operations:
Generally in good shape apart from some issues with the meaning of 
'regular files'.

s17: Security:
I have some doubts about the Kerberos algorithms suggested.  Maybe 
should be moving on to better algorithms even if this is a bis.

=============
Minor issues:
S1.1, bullet 7: Are there issues with backwards compatibility after 
removal of LIPKEY and SPKM-3 completely?

s1.5.1: Should NFSv4 be using RPCSEC_GSS v2 (RFC 5403) rather than v1 
(RFC 2203)?

s.1.5.3.2, para 4, and s5.3: It would be useful to say upfront in 
s1.5.3.2 whether the attribute names are fully internationalized or 
whether this is still an ASCII name.

s2.1:  This section contains a number of symbolic constants (chiefly for 
opaque element sizes) which have not been defined as yet.  It would be 
highly desirable if these were also defined before this section.

s2.1, utf8val_RECOMMENDED4:
>   String SHOULD be sent UTF-8
What else could it be sent as in this type?

s2.2/s6.2.1:  The type nfsace4 is used in the protocol: Accordingly it 
ought to be defined in s2.2 rather than s6.2.1.

s2.2.3/s5.8.2.34/s5.8.2.40: It would probably be nicer if the union case 
SET_TO_SERVER_TIME4 were put into the union explicitly.  A note about 
being able to set the access and modify times to either client or server 
time would be appropriate in the two items in s5.8.2.  Are there any 
constraints on the values that can be given when setting from the 
client.. like not moving the time backwards (or is this explained later?).

s3.1, para 1:
>   Using the registered port for NFS services
>     means the NFS client will not need to use the RPC binding protocols
>     as described in [RFC1833  <http://tools.ietf.org/html/rfc1833>]; this will allow NFS to transit firewalls.
I think some explanation of why this will definitively allow NFS to 
transit firewalls is needed.

s3.1.1: Presumably the requirement to eschew silent dropping of 
'requests' does not apply if security is being negotiated.  I take it 
'requests' should be interpreted as  'NFSv4 requests' and procedures as 
defined in s14/15 and this requirement should apply only once the secure 
connection is established.  The word 'established' might help.

s3.2: (reprise) Should we be using RPCSEC_GSSv2 (RFC 5203) rather than v1?

s3.2.1.1: The Kerberos specifications appear to use rather ancient 
algorithms.  In particular DES, MD5.. should there not be more modern 
algorithms?  DES and MD5 get a pretty bad press these days. RFC 2623 is 
also a bit of an antique so I can't say its a modern assessment.

s3.3.1,security poiicy boundaries etc:
>   In general, the client will not have to use the SECINFO
>     operation except during initial communication with the server or when
>     the client crosses policy boundaries at the server.  It is possible
>     that the server's policies change during the client's interaction
>     therefore forcing the client to negotiate a new security triple.
  The  concept of 'security policy boundary' has not been previously 
discussed in the doc.  How would the client know it had crossed a 
security boundary or that the server had changed the policy under its 
feet? I suppose maybe the next section tells me... but... How does this 
interact with the no silent drop, mostly no retries requirement in 
s3.1.1? or have i to read s15.33.

I suspect that some discussion of the interaction of mount points and 
security policy boundaries may be needed at an earlier stage.  I suspect 
that mount points might well be security policy boundaries and crossing 
mount points might interact.  There is nothing in the description of 
mounted_on_fileid (s5.8.2.19).

s5.6, item lease_time:  I can't find a definition of nfs_lease4 type 
used for this.

s5.6, item rdattr_error: Just giving 'enum' as the type isn't too specific!

s5.8.2.25:
> It is understood that this space may be
>     consumed by allocations to other files or directories though there is
>     a rule as to which other files or directories.
Where do I find the rule?

s5.10, para 1:
> although there are variations that occur additional characters with a
>     name including "SMALL" or "CAPITAL" are added in a subsequent version
>     of Unicode.
so... should the client be able to find out which version of Unicode the 
server is working to?

s6.3.1.2: Is there any interaction between delegation and client 
considerations for ACLs? Do clients holding delegations have to do any 
access checks in the client that would normally be done in the server if 
there was no delegation?  I'm not sure.

s7.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.
How is the verification achieved?  I don't see any attributes within 
NFSv4 that would help.

s7.7.6.1, para 3:
>     If state has not been transferred transparently because the client ID
>     is rejected when presented to the new server, the client should fetch
>     the value of lease_time on the new (i.e., destination) server, and
>     use it for subsequent locking requests.  However, the server must
>     respect a grace period of at least as long as the lease_time on the
>     source server, in order to ensure that clients have ample time to
>     reclaim their lock before potentially conflicting non-reclaimed locks
>     are granted.
How does the destination server know what the lease_time/grace period on 
the source server was? It doesn't know what server the client was 
previously connected to in many cases and so can't ask. Doesn't this 
amount to saying all servers for a given file system should (MUST) be 
configured with the same lease time in the non-transparent transparent case?

s9.9: This section is a little skimpy.  There are several points that 
probably ought to be clarified:
- The section explicitly refers to files:  Does it actually apply to all 
filesystem objects?
- An explicit statement that the 'file_state' is maintained for each 
file(system object)
- The pseudo-code applies to the OPEN case; different pseudo-code 
(presumably) applies to OPEN_DOWNGRADE.  I *think* I could guess it but 
it would be good to be explicit.
- Is there any interaction between byte range locks and share 
reservations?  e.g., does a client holding (say) a byte range write lock 
but with no DENY state prevent another client taking out a DENY_WRITE  
share reservation on the same file?
- Does the discussion of placing lock state into stable storage across 
server reboot also apply to the share reservation file_state?

s9.14:  The text in this section addresses topics also covered in s7.  
This does  not seem desirable as there may be discrepancies (see next 
comment).  It would seem better to merge the text into s7 and possibly 
provide a reference back to s7 from s9.

s9.14.1:  This section envisages migration of state from one server to 
another where the client already has state on the other server using the 
same client ID.  Merging of the states is suggested. AFAICS this 
situation is not discussed in s7 which seems a little odd.

s12:  It seems the client has to play '20 questions' with the server to 
find out how it treats internationalized component names.  Some of it 
seems little better than guesswork.  I was wondering why some of the 
server choices are not described using attributes so that the client 
doesn't have to thrash around in the dark.  Also there doesn't seem to 
be any requirement for a filesystem to be treated identically after 
migration. As far as I can see the client can't really rely on the 
internationalisation treatment not changing after migration.

s18.1:  The NFSv4 Named Attribute Definitions registry has already been 
created in exactly the form specified here by RFC 5661 (NFSv4.1).  This 
section should be removed and words noting that the existing registry is 
common to all minor versions of NFSv4  should be substituted.

==================
Nits/editorial comments:
General: Review the remaining instances of 'file' and 'files' to 
determine if they should really be 'filesystem objects'.

General: Consistency between 'file system' and 'filesystem'. 
'filesystem' is used first in abstract and s1.3 but only File System is 
defined.  I cannot see whether there is any real distinction between the 
usages... but  am open to argument! OTOH there is definitely 
inconsistency: Consider 'fsid' and 'fsid4' - the type definition says 
fsid4 is for a filesystem (s2.2.5) but s5.8.1.9 says this is about a 
'file system'

General: The terms 'regular file' and 'named attribute' are not used 
uniformly.  s5.8.1.2 typifies the confusion.  In the first set of 
bullets, NF4REG is defined as 'a regular file' and NF4NAMEDATTR is 
defined as a Named Attribute.  In the last bullet, "is an[sic] regular 
file" is defined as implying either type NF4REG or NF4NAMEDATTR.
Thus we have the same term used for a set and a superset of the same set 
(maybe when qualified with 'is a').  Consequently, for example in 
s15.25.4,  we have "the READ operation reads data from the regular 
file...".  Presumably this is supposed to cover both NF4REG and 
NF4NAMEDATTR types but one can't be absolutely sure (to be pedantic it 
doesn't explicitly say 'is a regular file' so perhaps it should be read 
as NF4REG only).  OTOH the OPEN operation (s15.18.5) has two separate 
paragraphs for regular files and named attributes. This needs some 
tidying up.  One could try using 'data object' as an unambiguous term 
for the union of the actual file (NF4REG) class and the named attribute 
class.

Abstract: s/owes heritage to/builds on the heritage of/

s1.1: Expand IDNA acronym.

s1.2, bullet 5:s/mounted_on_filed/mounted_on_fileid/

s1.3: Expand acronyms ONCRPC and RPCSEC_GSS (and/or ref to RFC 2203 or 
5403).

s1.5: s/the reader that is new/the reader who is new/

s1.5.1, para 2: s/provides the client a method/provides the client with 
a method/

s1.5.3:  A couple of sentences outlining the presentational scheme for 
filesystem object names (i.e., /a/b/c etc) would be worthwhile even if 
'we all know what this means'!  This could include defining 'component 
name' used later with no introduction.

s1.5.3.1, et seq: Would it be useful to identify 'persistent' and 
'volatile' as key words: e.g., 'Persistent' and 'Volatile'.

s1.5.3.2, para 3: For consistency the 'acl' example should have a ref 
(s6.2.1) earlier in the para.  So s/((Section 6)/(Section 6.2.1)/ and 
move one sentence earlier.

s1.5.5: There should be a discussion of share reservations in this section.

s1.6, definition of Lease: There should be a definition of 'grace 
period' (as per s9.6.2) as there are lots of references to it earlier 
than s9.6.2.

s1.6, definition of Lock:  There should be a definition of share 
reservation.

s2.1 et seq:  Whilst the terms 'hard link' and 'symbolic link' are 
pretty commonly known,  they are so fundamental that I think putting 
them in the definitions would be highly desirable.

s2.1, attrlist4:  In line with comment about opaque max sizes, should 
this have a max size? (Might also apply to bitmap4)

s2.2: It would be useful to have section references to the places where 
the types are used where these are explicitly mentioned.

s2.2.8 (and s2.1):  I guess that the bit ordering in bitmap4 is 
implicitly derived from the XDR representation, but it would be useful  
to remind people of what this bit ordering actually is.

s2.2.9, atomic element:  With a name like that are there any special 
handling requirements for this element/structure?

s2.2.10: Note  to self: verify external refs in this section.

s2.2.11, cb_program: It should be explained that the cb_program is an 
RPC program number (had to ferret in s15.35.4 to understand this).  BTW 
this would entail an earlier expansion of RPC acronym.

s2.2.12:  It would be good to have a definition of NFS4_OPAQUE_LIMIT 
before this section (see earlier comment about s2.1) so that all the 
magic number constants are concentrated in one place.

s2.2.13:  Arrgh! Now we have two definitions of NFS_OPAQUE_LIMIT 
(fortunately they are the same until they aren't).

s2.2.14: ARRRGGGHHH! THREE defs.

s2.2.16, 'other':  And this definition contains the magic number 12 not 
as a symbolic constant and with no explanation of why 12 is a good value!

s3.1, para 2:
>     Where an NFSv4 implementation supports operation over the IP network
>     protocol, the supported transports between NFS and IP MUST be among
>     the IETF-approved congestion control transport protocols, which
>     include TCP and SCTP.
I know what is meant, but technically, I don't believe the IETF actually 
has a list marked 'congestion control transport protocols'.  A better 
way of phrasing this might be:
    Where an NFSv4 implementation supports operation over the IP network
    protocol, the supported transport layer between NFS and IP MUST be 
an IETF
    standardised transport protocol that is specified to avoid network
    congestion; such  transports include TCP and SCTP.

[Check whether SCTP is on the IANA well-known acronym list].

Also:
OLD:
to use a different IETF-approved congestion control transport protocol.
NEW:
to use a different IETF standardised transport protocol with appropriate 
congestion control.

s3.1, para 3: Expand WAN acronym. (I think)

s3.1, para 4:  Where would I find a justification that DCCP or NORM are 
inadequate?

s3.1, para 5: s/regardless whether/regardless of whether/

s3.1, para 6: A rather throwaway comment about avoiding timer sync. Not 
sure yet how many timers, but it would be good to know which ones were 
critical.  After reding the document I am still not sure. It might 
almost be worth adding a section that summarizes the set of timers used.

s3.2.1/s3.2.1.1: Check section title capitalization.  (Mostly OK - 
spotted also s6.4.1/s6.4.1.x - possibly)

s4.2.1: Earlier parts of s4 have been generalised to talk about 
'filesystem objects'.  This section still has filehandles referring to 
'files'.

s4.2.3, para at top of p33; s4.3, para 4: Ditto talking about 'files'.

s4.3, para 1:
>     If possible, the client SHOULD recover from the receipt of an
>     NFS4ERR_FHEXPIRED error.
I think this is a 'should' rather than a 'SHOULD'. Its not something 
that the protocol designer has any control over.

s5 title:  Attributes apply to more than just files.

s5, para 4: s/new  OPENATTR operation/OPENATTR operation introduced in 
NFSv4/

s5.1, last sentence: I think the 'must' is a 'MUST' - the server is 
constrained to send the values if asked (but the client has a choice to 
ask or not).

s5.2:
> A client MAY ask for the
>     set of attributes the server supports and SHOULD NOT request
>     attributes the server does not support.
This should probably be
    An operation allows the client to determine the set of attributes
    that the server supports; a client should not request attributes the
    server does not support.
Its not something the protocol can do anything about.

s5.8.1.4:s/may/MAY/

s5.8.2.10, para 2: Not sure what root path has to do with locations.

s5.9, para 4:
>   Servers that do not provide support for all possible values of the
>     owner and owner_group attributes SHOULD return an error
>     (NFS4ERR_BADOWNER) when a string is presented that has no
>     translation, as the value to be set for a SETATTR of the owner,
>     owner_group, or acl attributes.
What might the server do if it doesn't return NFS4ERR_BADOWNER? I would 
have thought this ought to be  MUST.

s5.9, para 5:
>     The "dns_domain" portion of the owner string is meant to be a DNS
>     domain name.  For example,[email-masked].  Servers should accept
>     as valid a set of users for at least one domain.  A server may treat
>     other domains as having no valid translations.  A more general
>     service is provided when a server is capable of accepting users for
>     multiple domains, or for all domains, subject to security
>     constraints.
I suspect that the 'should' and 'may' ought to be 'SHOULD' and 'MAY'.  
Arguably the 'should' ought to be a 'MUST' if the server recognises 
owner and owner_group as attributes. i.e., a mandatory to implement.

s5.9, para 6:
> In the absence of such a
>     configuration, or as a default, the current DNS domain name should be
>     the value used for the "dns_domain".
The term 'current DNS domain' is not very specific. Do you mean the DNS 
domain in which the host resides?

s5.9, 1st bullet point on page 50: A reference to (probably) RFC 5890 is 
needed to explain U-labels and other IDN stuff is needed (probably best 
in s1.6).

s5.10: Expand UCS acronym (and add ref to ISO.10646-1.1993).

s6.1, bullet 3:  An explanation of what is meant by (acl) inheritance is 
needed.

s6.1, bullet 4, sub-bullet 1:
>        *  Setting only the mode attribute should effectively control the
>           traditional UNIX-like permissions of read, write, and execute
>           on owner, owner_group, and other.
Does this extend to the interpretation of the x bit as searchability on 
directory objects as per UNIX?

s6.1, bullet 5:  Does mode setting affect the inherited ACL attributes 
as well as the those of the object for which the mode is set?

s6.2.1, next to last para: Expand SMB (and/or give reference).

s6.2.1.1, sentence above table:
>     All four but types are permitted in the acl attribute.
'but' is clearly not the right word but I am not quite sure what this 
trying to say.

s6.2.1.3.1, ACE4_ADD_SUBDIRECTORY:
Why is the RENAME operation affected if the object being renamed is a 
file rather than a directory?

s6.2.1.3.1, ACE4_EXECUTE:
Given that directory operation aliases for ACE4_READ_DATA and 
ACE4_WRITE_DATA have been defined, it would seem sensible to define an 
alias for the directory/traverse use case of ACE4_EXECUTE.

s6.2.1.3.1, ACE4_DELETE_CHILD and ACE4_DELETE:
s/for information on/for information on how/

s6.2.1.3.1, ACE4_WRITE_ATTRIBUTES - file times modification:
Can the client find out if the server uses client or server times? I 
didn't see an attribute that tells me which is allowed on the server - 
cansettime only allows the client to find out if it can modify the 
times.  What happens (which error code comes back) if the SETATTR has 
the wrong sort of time spec (e.g., client time when the server only 
supports server times)? As previously mentioned, is the server expected 
to do any sanity checks on supplied client times (maybe not allowing - 
at least - ordinary users to set times backwards from previous values.)

s6.2.1.4: A few words on the effect of inheritance would help and and 
clarify the next two comments.  Points that arise from the discussions 
in s6.2.1.4.1:
- Is there a time factor in ACEs?  The phrase 'newly created' seems to 
imply that there is.  The document doesn't really explain how it is 
expected that inheritance will be implemented: It could be seen as 
copying the current set of ACEs from the parents up the tree at the time 
an object is created or tracking the ACEs up the tree when a access 
request has to be verified.
- Is the inheritance fully recursive down a directory tree?  I guess 
that it is unless the NO_PROPAGATE flag is set.. but what about symbolic 
links?
- Does the inheritance propagate across mount points?

Additionally, how does ACE inheritance interact with hard and symbolic 
links?  Does the inheritance attach to the underlying object or only to 
the name path?

s6.2.1.4.1, ACE4_FILE_INHERIT_ACE:
Three issues:
- presumably this implies the ACE is (primarily) inherited by files in 
the directory to which it applies
- does 'any sub-directory' imply that the inheritance applies 
recursively down the tree?
- does 'any non-directory file' include symbolic links?

s6.2.1.4.1, ACE4_DIRECTORY_INHERIT_ACE:
Three issues:
- Does 'should be added to each *new* directory created' imply that the 
ACE does not get inherited by sub-directories are already in existence 
when the ACE is applied? Either way it would be good to be explicit 
about the effect.
- Does this apply recursively down the tree?
- Would the inheritance apply to symbolic links to directories?

s6.2.2:
It would be worth pointing out that the execute permission bits imply 
searchability for directory objects.  This isn't mentioned in READDIR... 
is this something missing?

s6.2.2:
Are there restrictions on the mode bits that can be set on named 
attributes and their directories?  Wouldn't make a lot of sense to set 
the x bits on named attributes  (probably), nor the suid and svtx bits.

s6.3.1.2, para 1: s/interpretation the ACL/interpretation of the ACL/

s6.1, last para: Expand DCE/DFS acronyms.

s6.2: Expand TLS and SPKM acronyms. (IPsec is 'well-known')

s7.1:
> These attributes specify such file system
>     instances by specifying a server address target (either as a DNS name
>     representing one or more IP addresses or as a literal IP address)
>     together with the path of that file system within the associated
>     single-server namespace.
I don't understand 'associated single-server namespace'.  Does this mean 
the aggregated logical file system or something else?  Having read 
further I suspect that this at least needs a forward reference to Section 8.

s7.2 and s7.3:  These sections are very repetitive.  They essentially 
convey the fact that if a file system is absent only GETATTR on the 
absent system and READDIR on a  referring system will produce useful 
answers and only a couple of attributes are valid.

s7.6:
>     A potential problem exists if a client were to allow an open owner to
>     have state on multiple filesystems on server, in that it is unclear
>     how the sequence numbers associated with open owners are to be dealt
>     with, in the event of transparent state migration.  A client can
>     avoid such a situation, if it ensures that any use of an open owner
>     is confined to a single filesystem.
This paragraph is not very helpful because the concepts of 'open owner', 
associated state and sequence numbers have not been introduced up to 
this point apart from as cryptic references in type definitions.  At 
worst a forward reference is needed.  It might be better to have a brief 
introduction to the state mechanism under the NFSv4 basic concepts 
section (maybe around s1.5.4).

Without this introduction I have no idea under what circumstances the 
'open owner' might have state on multiple file systems - and hence what 
I would have to do to avoid going down this rat hole.

s7.7, last para:  Would be worthwhile to either substitute 'network 
order' for 'big endian' or add it as an alternative. (It turns out that 
s9.14 uses network order for just this requirement - consistency would 
be good!)

s7.7.1:
>     When a single file system may be accessed at multiple locations,
>     either because of an indication of file system identity as reported
>     by the fs_locations attribute, the client will, depending on specific
>     circumstances as discussed below, either:
The first 'either' has no corresponding 'or'.
The second 'either' needs an 'or' at the end of the first bullet.

s7.7.1, 2nd bullet: s/Accesses/Access/

s7.7.2, para 3 (and  para 2):
>     When there is co-operation in filehandle assignment, the two file
>     systems are reported as being in the same handle classes.
It appears that the term 'handle class' is not defined until s7.9.1 
(like fileid class in the s7.7.3 and various other attribute classes).  
A forward reference to this section is essential  (or maybe better a 
reordering of the sections).  It would be worth a note somewhere in this 
area that a client needs to ensure that it knows the fh_expire_type for 
any filesystem instance it is accessing  in case it get forcibly 
transitioned, since it can't find out post-facto from an absent filesystem.

s7.7.2, paras 2 and 3:
>     When there is no such cooperation in filehandle assignment, the two
>     file systems are reported as being in different handle classes.  In
>     this case, all filehandles are assumed to expire as part of the file
>     system transition.  Note that this behavior does not depend on
>     fh_expire_type attribute and depends on the specification of the
>     FH4_VOL_MIGRATION bit.
>
>     When there is co-operation in filehandle assignment, the two file
>     systems are reported as being in the same handle classes.  In this
>     case, persistent filehandles remain valid after the file system
>     transition, while volatile filehandles (excluding those that are only
>     volatile due to the FH4_VOL_MIGRATION bit) are subject to expiration
>     on the target server.
I do not understand the final parts of these paras where 
FH4_VOL_MIGRATION is mentioned:
- In para 2, FH4_VOL_MIGRATION is a bit in fh_expire_type, so I fail to 
understand how the behavior can both be independent of fh_expire_type 
and dependent on a bit within it at the same time.
- In para 3, the wording appears to imply that filehandles with 
FH4_VOL_MIGRATION set will not expire. My  understanding of the 
definition of this bit was that such filehandles would explicitly expire 
on migration.  Accordingly I would have expected that such filehandles 
would have been guaranteed to expire on transition  - but maybe this is 
some difficulty with the meanings of transition and migration?

s7.7.3, last para:
>     Note that when consistent fileids do not exist across a transition
>     (either because there is no continuity of fileids or because fileid
>     is not a supported attribute on one of instances involved), and there
>     are no reliable filehandles across a transition event (either because
>     there is no filehandle continuity or because the filehandles are
>     volatile),
I don't understand how there can be no filehandle continuity other than 
because the filehandle is volatile.  If a filehandle is not volatile, 
then it is persistent and s4.2.2 appears to indicate that any persistent 
filehandle would be valid across a transition.

s7.7.5 and s7.7.6, last paras:  s7.9.1 states that distinct server 
instances are always in different change classes. Since there doesn't 
seem to be anyway to check that two instances of a server are actually 
the same when accessed over different connections, the consistent change 
class mentioned in the last para of s7.7.5 and the continuity mentioned 
in the last para of s7.7.6 can never occur in practice and would 
probably be better omitted.

s7.7.6, para 7: A reference to s9.6.3.4 where the edge conditions are 
described would be desirable.

s7,7,8:  s7.9.1 says all server instances have different readdir 
classes, so once again the instances will never be deemed consistent.

s7.9:  Copying the fs_location(s)4 structure definitions here creates a 
double maintenance problem.  Better just to reference ss2.2.6/2.2.7 
where they are defined.

s8:  Placing this section much earlier in the document would help 
understanding, especially of Section 7.

s9:  There is a comment in s15.12.5 (para 2) that exactly how locks 
behave depends on the underlying semantics of the exported  file 
system.  It would be worth reproducing that statement here to ensure 
that this section is interpreted in that context.

s9, last para:
> To support Win32 share reservations it is necessary to atomically
>     OPEN or CREATE files.
Add 'and apply the appropriate locks in the same operation' to the end 
of this sentence.

s9.1: I think that for the sake of clarity, it would be worth explicitly 
stating that the client has to hold an open before applying locks to the 
same file.

s9.1:  What happens if you try to acquire a byte range lock on a 
non-file filesystem object?

s9.1.1, para after struct nfs_client_id4 definition: s/which the server 
has previously recorded the client/which the server has previously 
recorded for the client/.  Also copying the structure definition creates 
a double maintenance problem.  A ref to s2.2.12 would be better.

s9.1.3, first sentence:  Question: Should the 'including' list also have 
share reservations or are these included in byte-range locks or opens?

s9.1.3.5: Expand acronym I/O - this isn't on the RFC Editors 'well 
known' list - maybe strangely.

s9.1.3.2: NFS4_UINT32_MAX should be in the proposed constants section.

s9.1.3.2, para 2 and s9.1.6, para 1: The two paragraphs have conflicting 
statements about the initial value of seqid for locks (s9.1.3.2 => 1, 
s9.1.6 => server choice) - not that it really matters much unless 
clients check that they always get 1!  Note that s9.1.6 does say 
anything about missing out 0 when wrapping round.

s9.1.5, para 9 (bottom of page 116): s/ (e.g., another open specify 
denial of READs)/(e.g., another open specifying denial of READs)/

s9.4:  Changing:
OLD:
    The server is not
    required to maintain a list of pending blocked locks as it is used to
    increase fairness and not correct operation.
NEW:
    The server is not
    required to maintain a list of pending blocked locks as it is not 
used to
    provide correct operation but only to increase fairness.
scans more easily.

s9.6: If I understand correctly, share reservations with a DENY 
reservation are essentially locks that cover the whole file and adjust 
dynamically to cover the entire byte range of the file if it changes. 
Presumably. therefore, the whole discussion of lock recovery applies 
both to byte range locks and share reservation implicit locks.  I think 
it would be worth pointing out that this is the case at the beginning of 
s9.6 (assuming this is indeed the case - or alternatively explaining how 
things differ with share reservations).

s9.8, para 4: In the last sentence s/may not/MUST NOT/.

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.
I am not sure how you 'issue a pending I/O'.  Does this mean checking 
whether there is already a pending I/O request associated with this 
stateid?

s10.2, para 6: s/server times its wait/server times out its wait/

s10.2, para 7: I think the lat two sentences are requiremenst:
next to last sentence: s/may/MAY/
last sentence: s/should not/SHOULD NOT/

s10.2.1, para 9 (last para on p146):  I think that this para is 
referring to the server not the client: s/client/server/

s10.2.1, para 12 (2nd bullet): Good to have a ref to the Courtesy Locks 
section (s9.6.3.1)

s10.2.1, para 17:  "s special semantic adjustments" => either "special 
semantic adjustments" or "a special semantic adjustment"

s10.2.1, para 19:  It might be better to explain about the distinction 
between the CLAIM_xxx attribute to be used after server and client 
restarts that doesn't come till para 24 currently - the changeover to 
CLAIM_PREVIOUS in this para isn't obvious without this discussion.

s10.2.1, last para, sentence 1: s/Is/is/

s10.3, 1st bullet: s/and not change/and not the change attribute/ ... 
there are a couple of other instances where this might also be helpful.

s10.3, 2nd bullet: s/client OPENS as file/the client OPENS a file/

s10.3.3:  I need to think a bit more about what mandatory locking implies.

s10.4, para after 2nd set of bullets: should the 'standard stateid' be 
associated with the open-owner rather than the lock-owner as currently 
stated.

s10.4.3: Should there be some discussion of wrap round in the 'change' 
attribute?

s10.5.1, para 2: s/a complete cached copy  of file/a complete cached 
copy  of the file/,
s/In other case/In other cases/

s10.7, para 2:
OLD:

(regardless whether the file is local file or is being access remotely)

NEW:

(regardless of whether the file is local file or is being accessed remotely)

s11, item 8, first sentence: s/implement/inplemented/

s11, items 9 and 10: Its probably not that important, but would OPTIONAL 
<=> REQURED be allowed - i.e., two 'levels' of change rather then the 
explicit one currently documented.

s12.1.1, bullet 2: s/in many case/in many cases/

s12.1.2, para 1: s/important to understand/important to understanding/

s12.1.2: The quoted sections should be attributed to where they come 
from (including section numbers).

s12.1.2: There seems to be a spurious ACCENT in this para:
>           LATIN SMALL LETTER E, COMBINING MACRON, ACCENT, COMBINING ACUTE
>           ACCENT (U+0xxx, U+0304, U+0301)
s12.1.2 (Maybe expand IDNA again - its 180 pages since it was last 
expanded).

s12.2.3, para just after Table 5: s/or it in may/or it may/

s12.3: (maybe expand UCS again since it is 130 pages since previous 
occurrence).

s12.7, 2nd bullet: s/lintext4/linktext4/;  also the term 'link text' 
hasn't been properly defined.

s12.7.1.5.1: Need to expand NFC and NFD and provide a reference.

s12.7.1.5.2, para 8, last sentence: s/it should generally mapped/it 
should generally be mapped/

s12.7.3, bullet 4: Should 'converting a user to group to an internal id' 
be 'converting a user or group to an internal id'?

s13.1.1.7: s/fit/fitted/

s13.1.2.1 s/file handle/filehandle/

s13.1.2.7: should refer to filesystem object rather than file.

s13.1.4.2: Value of NFS4ERR_DQUOT is 19 here but 69 in table 8

s13.1.4.3/4/5/7: should refer to filesystem object rather than file.

s13.1.4.8: Does this apply to any filesystem object rather than just 
file or directory?

s13,1.4.11: Value of NFS4ERR_NXIO is 5 here but 6 in table 8 (and value 
5 is already used for NFS4ERR_IO).

s13.1.5.2: Value of NFS4ERR_BAD_STATEID is 10026 here but 10025 in table 
8 (and value 10026 is already used for NFS4ERR_BAD_SEQID).

s13.1.5.2, bullet 1: should refer to filesystem object rather than file.

s13.1.7: s/When the strings are not are of length zero./When the strings 
are not of length zero./

s13.2-13.4: I didn't check these tables.  Table 11 can be checked 
automatically if it is thought worthwhile by inverting tables 9 and 10.

s14.2: Might be worth stating that COMPOUND doesn't represent a 
'transaction', as is expressed in para 3.

s14.2, para 4:  The operation descriptions explicitly mention what 
happens to the current filehandle after operations.  Nothing is said 
about the stability of the saved filehandle.  Can this be assumed to be 
preserved in all cases whether the operation succeeds or fails? I guess 
it would also be useful to explicitly state that there are no guarantees 
about the contents of the current filehandle after an operation unless 
explicitly stated (i.e., after most errors you ought to reload the 
current filehandle).

s14.3, para 1: s/UNSTABLE/UNSTABLE4/

s15.2.5, para 2: I am not sure how you *could* mix operations - I was 
under the impression that the COMPOUND is submitted in the context of a 
particular client ID and the current/saved filehandles are tied to the 
client ID/connection. The client ID can't be changed in a single 
compound statement so this seems a spurious statement. Presumably if 
stateid's etc are used on the wrong client ID the server will object. 
See also s15.18.6.

s15.3.4: Presumably the access permissions generally apply to named 
attributes.  Currently it only talks about files and directories.

s15.3.5, para 2, sentence 3: s/which will result/which might result/?

s15.3.5, para 4: I found the following confusing:
>     The client should use the effective credentials of the user to build
>     the authentication information in the ACCESS request used to
>     determine access rights.
I am not sure where there is any authentication info (explicitly) "in 
the ACCESS request" - the parameter is just the requested access mask.  
Where is the auth info?

s15.9.4:
OLD:
> The server returns an attribute bitmap that
>     indicates the attribute values for which it was able to return,

NEW:
> The server returns an attribute bitmap that
>     indicates the attributes for which it was able to return values,

s15.9.5, last sentence: s/returned if while a delegation/returned if [or 
while] a delegation/

s15.11.4:  This section refers explicitly to files.  Is it ever possible 
to make hard links to:
- directories?
- named attributes?
- device and other special types of 'file'?
It would be good to be explicit.

s15.12.4, para 2:
>   To lock the file from a specific offset
>     through the end-of-file (no matter how long the file actually is) use
>     a length field with all bits set to 1 (one).
Is the end offset fixed at the value given by the length of the file at 
the time of applying the lock, or does this byte range vary as the 
length of the file changes?
What happens if the length of the file gets to be less than the start 
offset, or indeed is currently less than the length of the file?
Actually that is a more general question: Is any special treatment 
needed for locks that are outside the current length of the file? Does 
the server make any checks on whether a lock is within the current 
length of the file when applied - or automatically delete locks off the 
end of a file.. probably not but would be good to be explicit.

s15.14.5, para 1:

>     In all of
>         these cases, allowed by POSIX locking [fcntl  <cid:[email-masked]>] semantics, a client
>         receiving this error, should if it desires support for such
>         operations, simulate the operation using LOCKU on ranges
>         corresponding to locks it actually holds, possibly followed by LOCK
>         requests for the sub-ranges not being unlocked.

Shouldn't the client apply the sub-range locks before unlocking? 
Otherwise there is a potential race condition and some other client 
could get in a lock between the unlock and the relock. [After a bit of 
background reading I understand that this rather silly arrangement is 
'the way it is done'

s15.16.4/5:  What happens when you apply LOOKUPP to the 'hidden' named 
attributes directory?  Do you get NFS4ERR_NOENT or the filehandle of the 
object that owns the named attributes (inverting OPENATTR)?  s15.16.4 
only talks about the root of the server's file tree.

s15.18.6: Do SHARES work on named attributes?  Just checking...

s15.20.5, para3, sentence 1:  "Servers must not require..." I think this 
is a 'MUST NOT'.

s15.23.5: Acronm SNEGO needs an expansion and a reference (RFC 2478?)

s15.25.4, para 4: If the OPEN didn't use locks or share reservations and 
there is no delegation, is the stateid in the READ request the 'basic' 
OPEN stateid returned with all OPENs? It doesn't explicitly say this.  
(also relevant to WRITE operation - s15.38.4, para 3).

s15.25.5, para 1:
OLD:
>   It
>     is possible that the server reduce the transfer size and so return a
>     short read result.  Server resource exhaustion may also occur in a
>     short read.
NEW:
>   It
>     is possible that the server reduces the transfer size and so returns a
>     short read result.  Server resource exhaustion may also result in a
>     short read.

s15.28.4: The specification says nothing about the semantics of 
sub-directory removal.  NFSv3 says that 'some servers won't allow 
removal of non-empty directories'.  What is the story for NFSv4?  Is 
there any standardised strategy for dealing with 'orphaned' resources in 
sub-directories if the server does allow a non-empty sub-directory to be 
deleted?

s15.29.4, para 5:  It would be clearer to say that the operation will 
fail if the saved and current file handles refer to named attribute 
directories attached to different filesystem objects.

s15.30.5, para 1 , last sentence: s/such that is could/such that it could/

s15.33.5, para 2:
> The filehandle is either provided by the client (PUTFH,
>     PUTPUBFH, PUTROOTFH) or generated as a result of a name to filehandle
>     translation (LOOKUP and OPEN).
Aren't the PUTxxx filehandles supplied by the server?

s15.38.4:  Does all the discussion of writing to stable storage apply 
when writing to a named attribute?

s15.38.5, para next to next to last:
>     Some implementations may return NFS4ERR_NOSPC instead of
>     NFS4ERR_DQUOT when a user's quota is exceeded.  In the case that the
>     current filehandle is a directory, the server will return
>     NFS4ERR_ISDIR.  If the current filehandle is not a regular file or a
>     directory, the server will return NFS4ERR_INVAL.
  I am not sure what circumstances the last sentence is referring to.  
One interpretation would be that WRITE is not allowed at all for special 
files.  I guess this is not what is intended - please clarify.

---

From: Jari Arkko <[email-masked]>
Date: Thu, 30 May 2013 16:26:29 +0300

Elwyn and Martin - I can't thank you guys enough for your reviews. 

Authors, was there a response to Elwyn's review? I don't think I have seen one.

FWIW, I have raised a number of Discuss points based on Elwyn's, Martin's, and my own read of the document. Other ADs, including Pete and Joel, have also raised points either based on these reviews or their own perspectives but with similar topics that you guys raised.

Jari

On May 9, 2013, at 1:35 AM, Elwyn Davies <[email-masked]> wrote:

> This is a mammoth document.  The comments run to 650 lines.  Clearly there ae issues in modifying a -bis document given that it has to be pretty much compatible with its progenitor  - but that is now going on 14 years old. I believe that there are a significant number of minor issues to address and parts of the document (despite the overall size) need to provide a bit more explanation of what is going on.  In particular I think that a little bit more rationale over the way in which an NFS server attempts to provide something like a common interface to the two big player filesystem types (*ix and Windows) [Aside: I am disappointed that it doesn't try to handle versioning file systems like the venerable VAX filing system! ;-) ]  There are some areas where the addition of named attributes hasn't really been properly dealt with AFAICS. One major area that I felt ought to have been better addressed in the -bis was internationalisation.  As mentioned below, I don't understand why additional atrributes couldn't be added to help clients understand what the server is doing with internationalisation - the client has to do some complex acrobatics which seem close to guesswork to work out what is going on in the server.   As ever, I am not an expert in NFSv4 (although I used v2 extensively almost 30 years ago... arrrgghhh!) so there may well be comments where I don't understand the problem.
>       
> Major issues:
> 
> Combining two different access control paradigms (UNIX-clone mode control bitmasks and access control lists) is an uphill struggle and provides a deal of complexity in situations where both, either one or neither may be specified.  The detailed text deals reasonably with the various cases.  There are, in my view, a couple of significant problems:
> - The first is essentially editorial:  I would say that rather more than basic understanding of UNIX-like file systems is needed.  An explanation or at least a suitable reference to explain the use of the mode bits in s6.2.2, especially the first three (SUID, SGID, SVTX) would help, and a clearer linkage to the explanations of owner, group and other sets of users that are distributed in parts of the text.
> - The second is more fundamental:  The intended functionality of inheritance is not explicitly documented.  To come to any sort of understanding of how inheritance is supposed to work you have to read through to the next to the last section (s6.4.3).  I have now read through the section two or three times and I have a model in my mind, but there is no text that would allow me to verify my understanding.  I *think* i have understood that (1) inheritance only applies when you initially create an object rather than being fed through to all 'inferior' objects in the tree if a superior object has a heritable ACE added; (2) inheritance only applies if you don't explicitly give any ACEs when the object is created, and (3) if you give a mode but no ACL then some complex combination of inherited ACL and mode is applied.  An upfront explanation of what is trying to be achieved would have helped a lot.  However, none of this explains what happens with either hard or symbolic links.  It is unclear to me whether inheritance of ACLs is applied when creating a hard or symbolic link to an existing object.
> 
> This section is very verbose and contains a number of areas that bring in topics that  are actually forward references without indicating where the information can be found, particularly in relation to various 'change classes'.  Some reordering might help.  The whole concept of change classes seems a little odd, because when they are finally defined (s7.9.1), it appears that servers are rarely in the same class during any of the interesting transitions     (s7.9.1 says that in most cases they have to be taken as in different classes).  Overall, this section contains an awful lot of discursive specification relating to servers that may not have any coordination which makes it very difficult both from the specification and the implementation angles to check that everything is working.
> 
> s8, NFS Server namespace:  This is a important section for overall understanding and I think it would help to have it much earlier, certainly before s7.
> 
> Generally very well written, but the treatment of share reservations is very skimpy,
> It would also be good to be more 'honest' about the interaction with the semantics of the underlying file system as regards locks as described in s15.12.
> 
> s10, Clientside Caching:
> Clearly very complex to implement but seems to be well-described.  A bit of earlier explanation of the use of CLAIM_PREVIOUS vs CLAIM_DELEGATION_PREV in client/server reboots would help.
> 
> s11, Minor Versions:
> No particular issues.
> 
> A minefield.  There seems to be an awful lot of guess work for clients trying to decide how the sever behaves.  On reflection, I am surprised that several of the things that clients are supposed to deduce (e.g., what normalisation the server does) were not supplied as attributes of the server.  Is there some good reason for this?  I was also surprised that there didn't seem to be a requirement that servers working on a particular filesystem all did the same internationalisation things.  It appears to me that if a filesystem gets migrated the client doesn't have any sort of guarantee that the internationalisation behaviour won't change under its feet - and it has no easy way to check if this is or is not the case because of the lack of attributes.  I have no idea to what extent the internationalisation is implemented in current servers.
> 
> s13, Error Codes:
> Generally OK but with a few code value inconsistencies.  I haven't checked the cross-consistency of Table 9 and Table 11 or whether the sets of error codes per operation are really right - they are just too voluminous.
> 
> s14, NFSv4 Requests:
> No problems.
> 
> s15/16, Operations:
> Generally in good shape apart from some issues with the meaning of 'regular files'.
> 
> s17: Security:
> I have some doubts about the Kerberos algorithms suggested.  Maybe should be moving on to better algorithms even if this is a bis.
> 
> 
> =============
> Minor issues: 
> S1.1, bullet 7: Are there issues with backwards compatibility after removal of LIPKEY and SPKM-3 completely?
> 
> s1.5.1: Should NFSv4 be using RPCSEC_GSS v2 (RFC 5403) rather than v1 (RFC 2203)?
> 
> s.1.5.3.2, para 4, and s5.3: It would be useful to say upfront in s1.5.3.2 whether the attribute names are fully internationalized or whether this is still an ASCII name.
> 
> s2.1:  This section contains a number of symbolic constants (chiefly for opaque element sizes) which have not been defined as yet.  It would be highly desirable if these were also defined before this section.
> 
> s2.2/s6.2.1:  The type nfsace4 is used in the protocol: Accordingly it ought to be defined in s2.2 rather than s6.2.1.
> 
> s2.2.3/s5.8.2.34/s5.8.2.40: It would probably be nicer if the union case SET_TO_SERVER_TIME4 were put into the union explicitly.  A note about being able to set the access and modify times to either client or server time would be appropriate in the two items in s5.8.2.  Are there any constraints on the values that can be given when setting from the client.. like not moving the time backwards (or is this explained later?).
> 
> s3.1, para 1:
>>    as described in [
>> RFC1833]; this will allow NFS to transit firewalls.
> I think some explanation of why this will definitively allow NFS to transit firewalls is needed. 
> 
> s3.1.1: Presumably the requirement to eschew silent dropping of 'requests' does not apply if security is being negotiated.  I take it 'requests' should be interpreted as  'NFSv4 requests' and procedures as defined in s14/15 and this requirement should apply only once the secure connection is established.  The word 'established' might help.
> 
> s3.2.1.1: The Kerberos specifications appear to use rather ancient algorithms.  In particular DES, MD5.. should there not be more  modern algorithms?  DES and MD5 get a pretty bad press these days.  RFC 2623 is also a bit of an antique so I can't say its a modern assessment.
> 
>  The  concept of 'security policy boundary' has not been previously discussed in the doc.  How would the client know it had crossed a security boundary or that the server had changed the policy under its feet? I suppose maybe the next section tells me... but... How does this interact with the no silent drop, mostly no retries requirement in s3.1.1? or have i to read s15.33.    
> 
> I suspect that some discussion of the interaction of mount points and security policy boundaries may be needed at an earlier stage.  I suspect that mount points might well be security policy boundaries and crossing mount points might interact.  There is nothing in the description of mounted_on_fileid (s5.8.2.19).
> 
> s5.6, item lease_time:  I can't find a definition of nfs_lease4 type used for this.
> 
> so... should the client be able to find out which version of Unicode the server is working to?
> 
> s6.3.1.2: Is there any interaction between delegation and client considerations for ACLs? Do clients holding delegations have to do any access checks in the client that would normally be done in the server if there was no delegation?  I'm not sure.
> 
> s7.5:
> How is the verification achieved?  I don't see any attributes within NFSv4 that would help.
> 
> s7.7.6.1, para 3: 
> How does the destination server know what the lease_time/grace period on the source server was? It doesn't know what server the client was previously connected to in many cases and so can't ask.  Doesn't this amount to saying all servers for a given file system should (MUST) be configured with the same lease time in the non-transparent transparent case?
> 
> s9.9: This section is a little skimpy.  There are several points that probably ought to be clarified:
> - The section explicitly refers to files:  Does it actually apply to all filesystem objects?
> - An explicit statement that the 'file_state' is maintained for each file(system object)
> - The pseudo-code applies to the OPEN case; different pseudo-code (presumably) applies to OPEN_DOWNGRADE.  I *think* I could guess it but it would be good to be explicit.
> - Is there any interaction between byte range locks and share reservations?  e.g., does a client holding (say) a byte range write lock but with no DENY state prevent another client taking out a DENY_WRITE  share reservation on the same file?
> - Does the discussion of placing lock state into stable storage across server reboot also apply to the share reservation file_state?
> 
> s9.14:  The text in this section addresses topics also covered in s7.  This does  not seem desirable as there may be discrepancies (see next comment).  It would seem better to merge the text into s7 and possibly provide a reference back to s7 from s9.  
> 
> s9.14.1:  This section envisages migration of state from one server to another where the client already has state on the other server using the same client ID.  Merging of the states is suggested.  AFAICS this situation is not discussed in s7 which seems a little odd.
> 
> s12:  It seems the client has to play '20 questions' with the server to find out how it treats internationalized component names.  Some of it seems little better than guesswork.  I was wondering why some of the server choices are not described using attributes so that the client doesn't have to thrash around in the dark.  Also there doesn't seem to be any requirement for a filesystem to be treated identically after migration. As far as I can see the client can't really rely on the internationalisation treatment not changing after migration.
> 
> s18.1:  The NFSv4 Named Attribute Definitions registry has already been created in exactly the form specified here by RFC 5661 (NFSv4.1).  This section should be removed and words noting that the existing registry is common to all minor versions of NFSv4  should be substituted.
> 
> ==================
> Nits/editorial comments:
> General: Review the remaining instances of 'file' and 'files' to determine if they should really be 'filesystem objects'.
> 
> General: Consistency between 'file system' and 'filesystem'.  'filesystem' is used first in abstract and s1.3 but only File System is defined.  I cannot see whether there is any real distinction between the usages... but  am open to argument! OTOH there is definitely inconsistency: Consider 'fsid' and 'fsid4' - the type definition says fsid4 is for a filesystem (s2.2.5) but s5.8.1.9 says this is about a 'file system'
> 
> General: The terms 'regular file' and 'named attribute' are not used uniformly.  s5.8.1.2 typifies the confusion.  In the first set of bullets, NF4REG is defined as 'a regular file' and NF4NAMEDATTR is defined as a Named Attribute.  In the last bullet, "is an[sic] regular file" is defined as implying either type NF4REG or NF4NAMEDATTR.  
> Thus we have the same term used for a set and a superset of the same set (maybe when qualified with 'is a').  Consequently, for example in s15.25.4,  we have "the READ operation reads data from the regular file...".  Presumably this is supposed to cover both NF4REG and NF4NAMEDATTR types but one can't be absolutely sure (to be pedantic it doesn't explicitly say 'is a regular file' so perhaps it should be read as NF4REG only).  OTOH the OPEN operation (s15.18.5) has two separate paragraphs for regular files and named attributes.  This needs some tidying up.  One could try using 'data object' as an unambiguous term for the union of the actual file (NF4REG) class and the named attribute class.
> 
> s1.3: Expand acronyms ONCRPC and RPCSEC_GSS (and/or ref to RFC 2203 or 5403).
> 
> s1.5.1, para 2: s/provides the client a method/provides the client with a method/
> 
> s1.5.3:  A couple of sentences outlining the presentational scheme for filesystem object names (i.e., /a/b/c etc) would be worthwhile even if 'we all know what this means'!  This could include defining 'component name' used later with no introduction.
> 
> s1.5.3.1, et seq: Would it be useful to identify 'persistent' and 'volatile' as key words: e.g., 'Persistent' and 'Volatile'. 
> 
> s1.5.3.2, para 3: For consistency the 'acl' example should have a ref (s6.2.1) earlier in the para.  So s/((Section 6)/(Section 6.2.1)/ and move one sentence earlier.
> 
> s1.6, definition of Lease: There should be a definition of 'grace period' (as per s9.6.2) as there are lots of references to it earlier than s9.6.2.
> 
> s1.6, definition of Lock:  There should be a definition of share reservation.
> 
> s2.1 et seq:  Whilst the terms 'hard link' and 'symbolic link' are pretty commonly known,  they are so fundamental that I think putting them in the definitions would be highly desirable.
>  
> s2.1, attrlist4:  In line with comment about opaque max sizes, should this have a max size? (Might also apply to bitmap4)
> 
> s2.2: It would be useful to have section references to the places where the types are used where these are explicitly mentioned.
> 
> s2.2.8 (and s2.1):  I guess that the bit ordering in bitmap4 is implicitly derived from the XDR representation, but it would be useful  to remind people of what this bit ordering actually is. 
> 
> s2.2.9, atomic element:  With a name like that are there any special handling requirements for this element/structure?
> 
> s2.2.11, cb_program: It should be explained that the cb_program is an RPC program number (had to ferret in s15.35.4 to understand this).  BTW this would entail an earlier expansion of RPC acronym.
> 
> s2.2.12:  It would be good to have a definition of NFS4_OPAQUE_LIMIT before this section (see earlier comment about s2.1) so that all the magic number constants are concentrated in one place.
> 
> s2.2.13:  Arrgh! Now we have two definitions of NFS_OPAQUE_LIMIT (fortunately they are the same until they aren't).
> 
> s2.2.16, 'other':  And this definition contains the magic number 12 not as a symbolic constant and with no explanation of why 12 is a good value! 
> 
> s3.1, para 2:  
> I know what is meant, but technically, I don't believe the IETF actually has a list marked 'congestion control transport protocols'.  A better way of phrasing this might be:
>    protocol, the supported transport layer between NFS and IP MUST be an IETF
> to use a different IETF standardised transport protocol with appropriate congestion control.
> 
> s3.1, para 4:  Where would I find a justification that DCCP or NORM are inadequate?
> 
> s3.1, para 6: A rather throwaway comment about avoiding timer sync.  Not sure yet how many timers, but it would be good to know which ones were critical.  After reding the document I am still not sure.  It might almost be worth adding a section that summarizes the set of timers used.
> 
> s3.2.1/s3.2.1.1: Check section title capitalization.  (Mostly OK - spotted also s6.4.1/s6.4.1.x - possibly)
> 
> s4.2.1: Earlier parts of s4 have been generalised to talk about 'filesystem objects'.  This section still has filehandles referring to 'files'.
> 
> I think this is a 'should' rather than a 'SHOULD'. Its not something that the protocol designer has any control over.
> 
> s5, para 4: s/new  OPENATTR operation/OPENATTR operation introduced in NFSv4/
> 
> s5.1, last sentence: I think the 'must' is a 'MUST' - the server is constrained to send the values if asked (but the client has a choice to ask or not).
> 
> s5.2: 
>> A client MAY ask for the
> What might the server do if it doesn't return NFS4ERR_BADOWNER? I would have thought this ought to be  MUST.
> 
> s5.9, para 5:
>>    domain name.  For example, 
>> [email-masked]
>> .  Servers should accept
> I suspect that the 'should' and 'may' ought to be 'SHOULD' and 'MAY'.  Arguably the 'should' ought to be a 'MUST' if the server recognises owner and owner_group as attributes. i.e., a mandatory to implement.
> 
> s5.9, para 6:
>> In the absence of such a
> The term 'current DNS domain' is not very specific. Do you mean the DNS domain in which the host resides?  
> 
> s5.9, 1st bullet point on page 50: A reference to (probably) RFC 5890 is needed to explain U-labels and other IDN stuff is needed (probably best in s1.6).
> 
> s6.1, bullet 3:  An explanation of what is meant by (acl) inheritance is needed.
> 
> Does this extend to the interpretation of the x bit as searchability on directory objects as per UNIX?  
> 
> s6.1, bullet 5:  Does mode setting affect the inherited ACL attributes as well as the those of the object for which the mode is set?
> 
> 'but' is clearly not the right word but I am not quite sure what this trying to say.
> 
> Why is the RENAME operation affected if the object being renamed is a file rather than a directory?
> 
> Given that directory operation aliases for ACE4_READ_DATA and ACE4_WRITE_DATA have been defined, it would seem sensible to define an alias for the directory/traverse use case of ACE4_EXECUTE.
> 
> Can the client find out if the server uses client or server times? I didn't see an attribute that tells me which is allowed on the server - cansettime only allows the client to find out if it can modify the times.  What happens (which error code comes back) if the SETATTR has the wrong sort of time spec (e.g., client time when the server only supports server times)? As previously mentioned, is the server expected to do any sanity checks on supplied client times (maybe not allowing - at least - ordinary users to set times backwards from previous values.)
> 
> s6.2.1.4: A few words on the effect of inheritance would help and and clarify the next two comments.  Points that arise from the discussions in s6.2.1.4.1:
> - Is there a time factor in ACEs?  The phrase 'newly created' seems to imply that there is.  The document doesn't really explain how it is expected that inheritance will be implemented: It could be seen as copying the current set of ACEs from the parents up the tree at the time an object is created or tracking the ACEs up the tree when a access request has to be verified. 
> - Is the inheritance fully recursive down a directory tree?  I guess that it is unless the NO_PROPAGATE flag is set.. but what about symbolic links?
> Additionally, how does ACE inheritance interact with hard and symbolic links?  Does the inheritance attach to the underlying object or only to the name path?
> 
> - presumably this implies the ACE is (primarily) inherited by files in the directory to which it applies
> - does 'any sub-directory' imply that the inheritance applies recursively down the tree?
> - Does 'should be added to each *new* directory created' imply that the ACE does not get inherited by sub-directories are already in existence when the ACE is applied? Either way it would be good to be explicit about the effect.
> It would be worth pointing out that the execute permission bits imply searchability for directory objects.  This isn't mentioned in READDIR... is this something missing?
> 
> s6.2.2:
> Are there restrictions on the mode bits that can be set on named attributes and their directories?  Wouldn't make a lot of sense to set the x bits on named attributes  (probably), nor the suid and svtx bits.
> 
> I don't understand 'associated single-server namespace'.  Does this mean the aggregated logical file system or something else?  Having read further I suspect that this at least needs a forward reference to Section 8.
> 
> s7.2 and s7.3:  These sections are very repetitive.  They essentially convey the fact that if a file system is absent only GETATTR on the absent system and READDIR on a  referring system will produce useful answers and only a couple of attributes are valid.
> 
> s7.6:
> This paragraph is not very helpful because the concepts of 'open owner', associated state and sequence numbers have not been introduced up to this point apart from as cryptic references in type definitions.  At worst a forward reference is needed.  It might be better to have a brief introduction to the state mechanism under the NFSv4 basic concepts section (maybe around s1.5.4).
> 
> Without this introduction I have no idea under what circumstances the 'open owner' might have state on multiple file systems - and hence what I would have to do to avoid going down this rat hole.
> 
> s7.7, last para:  Would be worthwhile to either substitute 'network order' for 'big endian' or add it as an alternative. (It turns out that s9.14 uses network order for just this requirement - consistency would be good!)
> 
> s7.7.1:
> It appears that the term 'handle class' is not defined until s7.9.1 (like fileid class in the s7.7.3 and various other attribute classes).  A forward reference to this section is essential  (or maybe better a reordering of the sections).  It would be worth a note somewhere in this area that a client needs to ensure that it knows the fh_expire_type for any filesystem instance it is accessing  in case it get forcibly transitioned, since it can't find out post-facto from an absent filesystem.
> 
> s7.7.2, paras 2 and 3:
> I do not understand the final parts of these paras where FH4_VOL_MIGRATION is mentioned:
> - In para 2, FH4_VOL_MIGRATION is a bit in fh_expire_type, so I fail to understand how the behavior can both be independent of fh_expire_type and dependent on a bit within it at the same time.
> - In para 3, the wording appears to imply that filehandles with FH4_VOL_MIGRATION set will not expire. My  understanding of the definition of this bit was that such filehandles would explicitly expire on migration.  Accordingly I would have expected that such filehandles would have been guaranteed to expire on transition  - but maybe this is some difficulty with the meanings of transition and migration?
> 
> s7.7.3, last para:
> I don't understand how there can be no filehandle continuity other than because the filehandle is volatile.  If a filehandle is not volatile, then it is persistent and s4.2.2 appears to indicate that any persistent filehandle would be valid across a transition.
> 
> s7.7.5 and s7.7.6, last paras:  s7.9.1 states that distinct server instances are always in different change classes. Since there doesn't seem to be anyway to check that two instances of a server are actually the same when accessed over different connections, the consistent change class mentioned in the last para of s7.7.5 and the continuity mentioned in the last para of s7.7.6 can never occur in practice and would probably be better omitted.
> 
> s7.7.6, para 7: A reference to s9.6.3.4 where the edge conditions are described would be desirable.
> 
> s7,7,8:  s7.9.1 says all server instances have different readdir classes, so once again the instances will never be deemed consistent.
> 
> s7.9:  Copying the fs_location(s)4 structure definitions here creates a double maintenance problem.  Better just to reference ss2.2.6/2.2.7 where they are defined.
> 
> s8:  Placing this section much earlier in the document would help understanding, especially of Section 7.
> 
> s9:  There is a comment in s15.12.5 (para 2) that exactly how locks behave depends on the underlying semantics of the exported  file system.  It would be worth reproducing that statement here to ensure that this section is interpreted in that context. 
> 
> s9, last para:
> Add 'and apply the appropriate locks in the same operation' to the end of this sentence.
> 
> s9.1: I think that for the sake of clarity, it would be worth explicitly stating that the client has to hold an open before applying locks to the same file.
> 
> s9.1:  What happens if you try to acquire a byte range lock on a non-file filesystem object?
> 
> s9.1.1, para after struct nfs_client_id4 definition: s/which the server has previously recorded the client/which the server has previously recorded for the client/.  Also copying the structure definition creates a double maintenance problem.  A ref to s2.2.12 would be better.
> 
> s9.1.3, first sentence:  Question: Should the 'including' list also have share reservations or are these included in byte-range locks or opens?
> 
> s9.1.3.5: Expand acronym I/O - this isn't on the RFC Editors 'well known' list - maybe strangely. 
> 
> s9.1.3.2, para 2 and s9.1.6, para 1: The two paragraphs have conflicting statements about the initial value of seqid for locks (s9.1.3.2 => 1, s9.1.6 => server choice) - not that it really matters much unless clients check that they always get 1!  Note that s9.1.6 does say anything about missing out 0 when wrapping round.
> 
> s9.1.5, para 9 (bottom of page 116): s/ (e.g., another open specify denial of READs)/(e.g., another open specifying denial of READs)/
> 
> s9.4:  Changing:
> OLD:
>    The server is not
>    required to maintain a list of pending blocked locks as it is not used to
> s9.6: If I understand correctly, share reservations with a DENY reservation are essentially locks that cover the whole file and adjust dynamically to cover the entire byte range of the file if it changes. Presumably. therefore, the whole discussion of lock recovery applies both to byte range locks and share reservation implicit locks.  I think it would be worth pointing out that this is the case at the beginning of s9.6 (assuming this is indeed the case - or alternatively explaining how things differ with share reservations).
> 
> I am not sure how you 'issue a pending I/O'.  Does this mean checking whether there is already a pending I/O request associated with this stateid? 
> 
> s10.2.1, para 9 (last para on p146):  I think that this para is referring to the server not the client: s/client/server/
> 
> s10.2.1, para 12 (2nd bullet): Good to have a ref to the Courtesy Locks section (s9.6.3.1)
> 
> s10.2.1, para 17:  "s special semantic adjustments" => either "special semantic adjustments" or "a special semantic adjustment"
> 
> s10.2.1, para 19:  It might be better to explain about the distinction between the CLAIM_xxx attribute to be used after server and client restarts that doesn't come till para 24 currently - the changeover to CLAIM_PREVIOUS in this para isn't obvious without this discussion. 
> 
> s10.3, 1st bullet: s/and not change/and not the change attribute/ ... there are a couple of other instances where this might also be helpful.
> 
> s10.4, para after 2nd set of bullets: should the 'standard stateid' be associated with the open-owner rather than the lock-owner as currently stated.
> 
> s10.4.3: Should there be some discussion of wrap round in the 'change' attribute?
> 
> s10.5.1, para 2: s/a complete cached copy  of file/a complete cached copy  of the file/,
> s11, items 9 and 10: Its probably not that important, but would OPTIONAL <=> REQURED be allowed - i.e., two 'levels' of change rather then the explicit one currently documented.
> 
> s12.1.2: The quoted sections should be attributed to where they come from (including section numbers).
> 
> s12.1.2 (Maybe expand IDNA again - its 180 pages since it was last expanded).
> 
> s12.3: (maybe expand UCS again since it is 130 pages since previous occurrence).
> 
> s12.7, 2nd bullet: s/lintext4/linktext4/;  also the term 'link text' hasn't been properly defined.
> 
> s12.7.1.5.2, para 8, last sentence: s/it should generally mapped/it should generally be mapped/
> 
> s12.7.3, bullet 4: Should 'converting a user to group to an internal id' be 'converting a user or group to an internal id'?
> 
> s13.1.1.7: s/fit/fitted/
> 
> s13.1.4.8: Does this apply to any filesystem object rather than just file or directory?
> 
> s13,1.4.11: Value of NFS4ERR_NXIO is 5 here but 6 in table 8 (and value 5 is already used for NFS4ERR_IO).
> 
> s13.1.5.2: Value of NFS4ERR_BAD_STATEID is 10026 here but 10025 in table 8 (and value 10026 is already used for NFS4ERR_BAD_SEQID).
> 
> s13.1.7: s/When the strings are not are of length zero./When the strings are not of length zero./
> 
> s13.2-13.4: I didn't check these tables.  Table 11 can be checked automatically if it is thought worthwhile by inverting tables 9 and 10.
> 
> s14.2: Might be worth stating that COMPOUND doesn't represent a 'transaction', as is expressed in para 3.
> 
> s14.2, para 4:  The operation descriptions explicitly mention what happens to the current filehandle after operations.  Nothing is said about the stability of the saved filehandle.  Can this be assumed to be preserved in all cases whether the operation succeeds or fails?  I guess it would also be useful to explicitly state that there are no guarantees about the contents of the current filehandle after an operation unless explicitly stated (i.e., after most errors you ought to reload the current filehandle).
> 
> s15.2.5, para 2: I am not sure how you *could* mix operations - I was under the impression that the COMPOUND is submitted in the context of a particular client ID and the current/saved filehandles are tied to the client ID/connection. The client ID can't be changed in a single compound statement so this seems a spurious statement.  Presumably if stateid's etc are used on the wrong client ID the server will object. See also s15.18.6.
> 
> s15.3.4: Presumably the access permissions generally apply to named attributes.  Currently it only talks about files and directories.
> 
> I am not sure where there is any authentication info (explicitly) "in the ACCESS request" - the parameter is just the requested access mask.  Where is the auth info?
> 
> s15.9.4:
> OLD:
> s15.9.5, last sentence: s/returned if while a delegation/returned if [or while] a delegation/
> 
> s15.11.4:  This section refers explicitly to files.  Is it ever possible to make hard links to:
> - directories?
> - named attributes?
> Is the end offset fixed at the value given by the length of the file at the time of applying the lock, or does this byte range vary as the length of the file changes?
> What happens if the length of the file gets to be less than the start offset, or indeed is currently less than the length of the file?
> Actually that is a more general question: Is any special treatment needed for locks that are outside the current length of the file?  Does the server make any checks on whether a lock is within the current length of the file when applied - or automatically delete locks off the end of a file.. probably not but would be good to be explicit. 
> 
> s15.14.5, para 1:
>> In all of
>>    these cases, allowed by POSIX locking [
>> fcntl
>> ] semantics, a client
> Shouldn't the client apply the sub-range locks before unlocking?  Otherwise there is a potential race condition and some other client could get in a lock between the unlock and the relock. [After a bit of background reading I understand that this rather silly arrangement is 'the way it is done'
> 
> s15.16.4/5:  What happens when you apply LOOKUPP to the 'hidden' named attributes directory?  Do you get NFS4ERR_NOENT or the filehandle of the object that owns the named attributes (inverting OPENATTR)?  s15.16.4 only talks about the root of the server's file tree. 
> 
> s15.20.5, para3, sentence 1:  "Servers must not require..." I think this is a 'MUST NOT'.
> 
> s15.25.4, para 4: If the OPEN didn't use locks or share reservations and there is no delegation, is the stateid in the READ request the 'basic' OPEN stateid returned with all OPENs? It doesn't explicitly say this.  (also relevant to WRITE operation - s15.38.4, para 3).
> 
> s15.25.5, para 1:
> OLD:
>>  It
> s15.28.4: The specification says nothing about the semantics of sub-directory removal.  NFSv3 says that 'some servers won't allow removal of non-empty directories'.  What is the story for NFSv4?  Is there any standardised strategy for dealing with 'orphaned' resources in sub-directories if the server does allow a non-empty sub-directory to be deleted?
> 
> s15.29.4, para 5:  It would be clearer to say that the operation will fail if the saved and current file handles refer to named attribute directories attached to different filesystem objects.
> 
> s15.38.4:  Does all the discussion of writing to stable storage apply when writing to a named attribute?
> 
>  I am not sure what circumstances the last sentence is referring to.  One interpretation would be that WRITE is not allowed at all for special files.  I guess this is not what is intended - please clarify.
> 
> 
> 
> 
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art

---

From: "Haynes, Tom" <[email-masked]>
Date: Thu, 30 May 2013 16:49:54 +0000

> Authors, was there a response to Elwyn's review? I don't think I have seen one.

I've got it in draft form. Too many items to tackle at once.

I'd like to see the larger discussion on i18n play out first as well.