Subject: [nfsv4] I-D Action: draft-ietf-nfsv4-xattrs-01.txt

From: [email-masked]
Date: Tue, 18 Aug 2015 22:09:11 -0700

A New Internet-Draft is available from the on-line Internet-Drafts directories.
 This draft is a work item of the Network File System Version 4 Working Group of the IETF.

        Title           : File System Extended Attributes in NFSv4
        Authors         : Manoj Naik
                          Marc Eshel
	Filename        : draft-ietf-nfsv4-xattrs-01.txt
	Pages           : 26
	Date            : 2015-08-18

Abstract:
   This document proposes extensions to the NFSv4 protocol which allow
   file extended attributes (hereinafter also referred to as xattrs) to
   be manipulated using NFSv4.  An xattr is a file system feature that
   allows opaque metadata, not interpreted by the file system, to be
   associated with files and directories.  Such support is present in
   many modern local file systems.  New file attributes are proposed to
   allow clients to query the server for xattr support, and new
   operations to get and set xattrs on file system objects are provided.

The IETF datatracker status page for this draft is:
https://datatracker.ietf.org/doc/draft-ietf-nfsv4-xattrs/

There's also a htmlized version available at:
https://tools.ietf.org/html/draft-ietf-nfsv4-xattrs-01

A diff from the previous version is available at:
https://www.ietf.org/rfcdiff?url2=draft-ietf-nfsv4-xattrs-01

Please note that it may take a couple of minutes from the time of submission
until the htmlized version and diff are available at tools.ietf.org.

Internet-Drafts are also available by anonymous FTP at:
ftp://ftp.ietf.org/internet-drafts/

---

From: Benny Halevy <[email-masked]>
Date: Thu, 27 Aug 2015 16:33:37 +0300

Hi Manoj,
.
I've reviewed the latest xattrs draft proposal.
Overall the draft looks good and I have a few minor comments
listed below.

Thanks,

Benny

* 7.1.1.  xattr4
- struct xattr4 is used only once as a member of struct SETXATTR4args.
  Therefore it's superfluous and can be "open coded" there.

- The section in this case will only hold the definitions of the
  basic data structures: xattrname4 and xattrvalue4.

- That said, the name part is referred to as "key" throughout the spec.
  Why not just call it unambiguously xattrkey4?

* 7.3.  New Operations
- Are all new ops independently optional or are they dependent and required
  to be implemented if xattr_support is set? (in an "all or nothing" fashion)

* 7.3.1.  GETXATTR
- nit: xdr field name prefixes seem a bit short to be globally unique.
  How about adding an "x", e.g. gxa_ for GETXATTR4 and gxr_ for the respective result?

- If the server does not support xattrs on the target file
   system, then it MUST NOT return a value and MUST return an error.

  Please suggest an error code (existing or a new one if required)

- The server also MUST return an error if it supports xattrs on the
   target but cannot obtain the requested data.  In that case, no value
   will be returned.

   a. Ditto error code. What are the different conditions where the server
      wouldn't be able to obtain the requested data?
      - generic conditions such as an I/O error are not needed to be expressed,
        however, e.g. when the requested attribute name is not found an explicit
        error code must be defined and listed here.

   b. The text seems contradictory, by returning no value you mean return an error
      and fall into the GETXATTR4res.default case? Or return a null gr_value?
      There is no need to reiterate that when an error is returned no value is returned.

- What are the access control requirements for this and other xattr operations?
  file read permission for getting and listing xattrs and file write permission
  for setting and removing xattrs?

* 7.3.2.  SETXATTR
- The change desired is specified by sa_type.
   SETXATTR4_CREATE is used to associate the given value with the given
   extended attribute key for the file system object specified by the
   current filehandle.  The server MUST return an error if the attribute
   key already exists.  SETXATTR4_REPLACE is also used to set an xattr,
   but the server MUST return an error if the attribute key does not
   exist.

  How does the client set an xattr without caring if it exists or not?
  The available types will cause failure if the attribute did not or did
  previously exist, respectively.

  -  How about adding a SETXATTR4_UNCONDITIONAL option?

  - nit: specifying these as "types" seems too broad.
    I'd rather define them as "options", i.e. enum setxattr_option4
    and sa_option.

  - Errors to be returned need to be defined here.

- If the xattr key and value contained in the client request are such
   that the request would exceed the channel's negotiated maximum
   request size, then the server MUST return NFS4ERR_REQ_TOO_BIG in
   sr_status.

  - I presume that this is repeated here for the sake of clarity to differentiate
    this case from exceeding filesystem limits on key/value length.  If so,
    I'd add a reference to RFC5661, on the lines of: "As specified in [RFC5661],
    if the xattr ... and so on.

- A successful SETXATTR MUST change the file time_modify and change
   attributes if the xattr is created or the value assigned to xattr
   changes.

  - What about time_metadata?

* 7.3.3.  LISTXATTR
- nit: since the operations returns a list of possibly multiple
  xattrs I'd call it LISTXATTRS in plural, rather than singular LISTXATTR
  (which implies operation on a single xattr)

- The returned cookie in lr_cookie is one per response,
  in contrast to READDIR4's which is one per returned entry.
  The latter allows the client to resume retrieval of the list
  on a per entry resolution.  Although the payload of LISTXATTR
  is supposed to be "small"-ish I imagine that the client may
  hit memory allocation constraints and may need to restart
  the list operation on any arbitrary entry.  Supporting that
  may look like this:

   /// struct lxr_entry (
   ///         nfs_cookie4    lxre_cookie;
   ///         xattrkey4      lxre_key;
   /// };
   /// struct LISTXATTR4resok (
   ///         verifier4      lr_cookieverf;
   ///         bool           lr_eof;
   ///         lxr_entry      lr_entries<>;
   /// };

  - If the server determines that the la_cookieverf is no
   longer valid for the directory, the error NFS4ERR_NOT_SAME must be
                        ^^^^^^^^^
   returned.

   - Copy/paste typo.  s/directory/list/

- Ideally, the cookie value should not change if the
   object xattr values is modified since the client may be caching these
   values.

  - nit: s/values/value/

  - The text is too permissive to be useful to a client.
    The reason the operation uses a verifier is to validate the
    previously retrieved cookies and that is independent of caching.
    Therefore an implementation guideline is to instruct nfs clients
    to cache both cookies and respective verifier.  Or refrain from
    caching listxattr cookies altogether beyond the span of the operation.

* 7.3.4.  REMOVEXATTR
- The server MUST return an error
   if the attribute key does not exist.

  - Ditto error code

- If the xattr key contained in
   the client request exceeds the channel's negotiated maximum request
   size, then the server MUST return NFS4ERR_REQ_TOO_BIG in rr_status.

  - No need to explicitly repeat this generic condition here.

- A successful REMOVEXATTR SHOULD change the file time_modify and
   change attributes.  However, these attributes SHOULD NOT be changed
   unless the xattr is not removed.

  a. time_metadata

  b. the latter sentence is superfluous unless there is a valid condition where
     the operation will succeed and the attribute not be removed.  Is there one?

On 2015-08-19 8:09, [email-masked] wrote:
> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4
>

---

From: David Noveck <[email-masked]>
Date: Thu, 27 Aug 2015 10:17:35 -0400

>  to be implemented if xattr_support is set? (in an "all or nothing"
fashion)

Good question.

I need to check draft-ietf-nfsv4-versioning-01 to see what it says about
requiring information like this in a feature specification document.  If it
doesn't now, the -02 will require it.

>  Please suggest an error code (existing or a new one if required)

I think NFS4ERR_NOTSUPP would be right.