Subject: [nfsv4] As promised, review of draft-ietf-nfsv4-xattrs-02

From: David Noveck <[email-masked]>
Date: Tue, 19 Apr 2016 14:21:57 -0400

*General Comments*

*Overall Evaluation*

Overall this document is in good shape.  A lot of the issues I've found are
basically editorial.

I expect that the technical issues I've found will be dealt with fairly
easily and we could then move forward to WGLC once the document receives
more working group review.

The things I'm worried about are:

   - The XDR difficulties dealt with below in *XDR Issues*.
   - The reorganization needed in *7.5. Modifications to Existing
   Operations*
   - I may just have missed this but I think the working group will need
   information on the state of prototype implementations as discussed in* WGLC
   and Prototype Implementations*

*XDR Issues*

I've made some suggestions on dealing with this in *6. XDR
Description* and *6.1.
Code Components Licensing Notice*. I think the result is sufficiently clear
that anyone likely to be implementing the extension has the information
necessary to produce the needed XDR. The potential problem is that it may
not be clear to the members of the IESG, who are not familiar with NFSv4 or
XDR. My worry is that they are going to complain about that and complain
that different readers could arrive at different XDR files.

*WGLC and Prototype Implementations*

Although this is not discussed within the document, one of the important
considerations at WGLC will be the state of prototype implementations.
Although, officially, interoperating implementations are not a requirement
for Proposed Standard, getting implementations done is one good way of
seeing if the spec has the necessary information to describe the protocol
adequately.

*Comments by Section*

*Abstract*

In the first sentence suggest:

   - replacing "extensions to" by "an OPTIONAL feature extending"
   - replacing "allow file" by "allows"
   - replacing "manipulated" by "interrogated and manipulated"

*1. Introduction*

In the first sentence of the first paragraph, suggest replacing "typically
organized in key/value pairs" by "organized as key/value pairs".

In the first sentence of the third paragraph, suggest replacing "portable
use" by "inclusion in NFSv4".

In the last sentence of the fourth paragraph, suggest replacing "NFS" by
"NFSv4".

*2, Use cases*

I suggest that the title of this section be changed to "Current and
Potential Uses of Extended Attributes"

In the last sentence of the third paragraph, suggest replacing "NFS" by
"NFSv4".

*3. File System Support*

I think we need to deal with forks and streams differently. I propose
replacing the fourth and fifth paragraphs by the following material:

Some file systems have facilities that are capable of storing both extended
attributes and named attributes. For discussion regarding the relationship
between these feature, see section 5.

   - Solaris 9 and later provides file "forks", logically represented
as files within a hidden directory that is associated with the target
file [fsattr].
   - In the NTFS file system, extended attributes may be stored within
"file streams" [NTFS].

*5.  Differences from Named Attributes*

Suggest renaming this section "Relationship with Named Attributes"

Suggest adding the following new paragraph after the existing fourth paragraph:

Despite these differences, the underlying file system structure used
to store named attributes are generally capable of storing xattrs.
However, the converse is typically not the case because of the size
limits applicable to xattrs.s.

*6.  XDR Description*

In the first sentence of the penultimate paragraph, suggest replacing
"The embedded" by "The initial section of the embedded"

Propose adding the following new paragraph, just before the start of
*6.1.  Code Components Licensing Notice*:

To produce a compilable XDR file, following procedure is suggested:

   - Extract the file nfs4_prot.x as described in [NFSv42-dot-x].
   - Extract xattr_prot.x from this document as described above.
   - Apply any changes required for other extensions to be included
together with the xattr extension.
   - Perform modifications to nfs4_prot.x as described by comments
within xattr_prot.x.
   - Extend the unions nfs_argop4 and nfs_resop4 to include cases for
the new operations defined in this document.
   - Combine the XDR files for the base v4.2 protocol, and all needed
extensions by either concatenating the relevant XDR files, or uing
file inclusion.

*6.1.  Code Components Licensing Notice*

Propose starting a new subsection after the end of the licensing
notice.  I suggest it be called *6.2 XDR for xattr extension.*

Within this new section, I suggest adding the following lines just
before the first %#include.

/// *

/// * The include named nfsv4.x should be extracted from the relevant
RFC and then

/// * modified to extend enums and unions as specified by this
extension and by other

/// * extensions to be supported.  .x files for other extensions may
also have to be

/// * included.

The order of the includes needs to be looked at It seems to me that they
should be reversed.

*7.  Protocol Extensions*

In the second sentence of the first paragraph, suggest replacing
"check if the server supports" by "determine if the file system being
accessed provides support for".

Suggest replacing the third sentence of the first paragraph by the
following suggested new paragraph:

In addition, extension are made to existing operations and attributes
as follows:

   - The ACCESS operation is extended by adding new mask bits to
provide access information relating to xattrs.
   - The acl attribute is extended by adding new bits to the ACE
access mask field to separately control permissions to query and
modify xattrs.

*7.2.  New Attribute*

A couple of problems:

   - "RECOMMENDED", while consistent with RFC5661, is not consistent
with RFC2119 or nfsv4-vers :-(
   - This doesn't really deal with the fact that some servers will not
be aware of this attribute (as opposed to simply not supporting it).

Here's a proposed replacement.

The per-fs read-only attribute described in Section 7.2.1 may be used
to determine if xattrs are supported.  Servers need not support this
attribute and some NFSv4.2 servers may be unaware of its existence.
Before interrogating this attribute using GETATTR, a client should
determine whether it is a supported attribute by interrogating the
supported_attrs attribute.

*7.4.  New Operations*

In the first sentence of the last paragraph, "RECOMMENDED" should be
replaced by "OPTIONAL"

The second sentence of the last paragraph is incorrect since the
xattr_support attribute can be supported and still return false.
Suggest the following replacement:

A server MUST support all four operations when they are directed to a
file system which supports the xattr_support attribute and returns
TRUE when it is interrogated.  For file systems which either do not
support the xattr_support attribute or which returns FALSE when it is
interrogated, all of these operations MUST NOT be supported.

To deal with the possibility of clients interacting with servers that have
no knowledge of the existence of this extension, suggest adding the
following paragraph just before *7.4.1. GETXATTR - Get an extended
attribute of a file*:

Clients need to be aware of the possibility that, if this feature is
implemented as an extension to an existing minor version, as described in
[nfsv4-vers]. they may interact with severs having no knowledge of the
existence of these new operations.  When interacting with such a server use
of any of these operation will result in an NFS4ERR_OP_ILLEGAL error or an
XDR decode error.

*7.4.1.3.   DESCRIPTION*

The second sentence of the second paragraph does not account for the
fact that the server may not be aware  of these new operations, and
thus cannot return NFS4ERR_NOTSUPP. I suggest the following
replacement be used:

If the server does not support xattrs on the target file system, then
it MUST NOT return a value.  In such cases, servers MAY return
NFS4ERR_OP_ILLEGAL or an XDR decode error but MUST return an
NFS4ERR_NOTSUPP error, if it can interpret the request.

*7.4.2.3.   DESCRIPTION*

In the second sentence of the third paragraph, need to replace "SHOULD
NOT" by "should not".  This seems to me like a quality of
implementation issue.

*7.4.3.   **LISTXATTRS - List extended attributes of a file*

I think this has already been discussed and agreed to on the list, but
it should be noted that ARGUMENTS, RESULTS, DESCRIPTION, and
IMPLEMENTATION all need to be prepared to eliminate reference to the
verifier.  BTW, *7.4.5.  Valid Errors* probably needs to be updated as
well.

*7.4.4.3.   DESCRIPTION*

In the second sentence of the second paragraph, need to replace
"SHOULD" by "MUST"

*7.5.  Modifications to Existing Operations*

There are a couple of significant issues to be addressed.  Both issues
apply to both ACCESS and acl.

The first concerns how to deal with updates to existing operations and
attributes (as opposed to new ones).

The idea (in nfsv4-vers) is to have a replacement section for each
modified operation or attribute, since if we didn't do that, there
would not be a single place to go to read a current description of
each attribute or operation.

So this would mean you would need an updated replacement for:

   - the description of ACCESS to replace section 18.1 in RFC5661.
   - the description of the acl attribute to replace section 6.2,1 in RFC5661.

I think Tom probably can help you with access to the existing XML
source for these to use as a base,

The other issue is that these updated descriptions would have to be
written indicating what happens when the feature is not supported or
not known about.

As long as clients might validly encounter servers unaware of those
extensions, the spec has to indicate that this is valid behavior.