Subject: [nfsv4] AD Review of draft-ietf-nfsv4-umask-03

From: Spencer Dawkins at IETF <[email-masked]>
Date: Fri, 28 Apr 2017 14:35:25 -0500

Dear Authors,

This draft is very easy to understand - thank you for that.

I'm fine requesting Last Call without this change, but if you could submit
a revision that defines ACE on first use after Last Call, that would be
nice.

More broadly,

I note that the xaddrs draft defines an xaddr_support attribute, but this
draft doesn't have anything like a umask_support attribute. That could be
fine (the algorithm looks like "try to set the umask, and if you get back
NFS4ERR_INVAL, it didn't work, doesn't matter why"), but I wonder about the
programming model being different between the first two variants for this
version ... I don't have an opinion about that, but wanted to ask before
they're both RFCs :-)

Thanks,

Spencer

---

From: David Noveck <[email-masked]>
Date: Fri, 28 Apr 2017 15:52:24 -0400

I'd like to explain why these two drafts appropriately made different
choices in this regard.

The xattrs draft defined a number of new operations, so it makes sense when
entering a file system to determine if those operations are supported for
that file system.  That attribute can be supported by the file system in
which case it tells you that xattrs are or are not supported or the
attribute might not be supported, in which case you can conclude, as the
default, that xattrs are not supported.

For umask, the extension consists of a single attribute and it doesn't add
anything (but complexity) to add second new attribute to tell you if the
first new attribute is supported.   For any attribute, you can find out if
it is supported within a given file system, by interrogating the
appropriate bit position in the REQUIRED attribute supported_attrs.

On Fri, Apr 28, 2017 at 3:35 PM, Spencer Dawkins at IETF <
[email-masked]> wrote:

> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4
>
>

---

From: "J. Bruce Fields" <[email-masked]>
Date: Fri, 28 Apr 2017 16:01:19 -0400

> a revision that defines ACE on first use after Last Call, that would be

Thanks, I'll take a look.

> version ... I don't have an opinion about that, but wanted to ask before

All this extension does is define a new attribute, and the NFSv4
protocol already includes a supported_attributes attribute which allows
a client to determine which attributes the server supports.  So I
believe a umask_support attribute would be redundant.

In fact, I think the xaddr_support attribute should probably be dropped
from the xattr draft.  The xattr draft doesn't define new any other new
attributes, but it does define new operations, so it's already possible
to query for support by trying an operation and checking the error
return (a NOTSUPP error if the filesystem lacks support, OP_ILLEGAL or
BADXDR (I think Noveck's version draft has details about this) if the
lack of support is server-side).

At a minimum if we keep the xaddr_support attribute then the xattr draft
needs to explain the motivation and clarify the attribute scope.  (I'm
not sure whether it's a filesystem- or server- wide attribute.)

--b.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Fri, 28 Apr 2017 16:29:25 -0400

> default, that xattrs are not supported.

You should also be able to do this by trying an operation and checking
for NFS4ERR_NOTSUPP, shouldn't you?

--b.

---

From: David Noveck <[email-masked]>
Date: Fri, 28 Apr 2017 17:54:07 -0400

> for NFS4ERR_NOTSUPP, shouldn't you?

It's not quite that simple.  In some cases you, could get an OP_ILLEGAL or
even an XDR decode error.   This is discussed in the last paragraph of
Section 8.4
of xattr-04.  You can still do the more complicated thing but the document
recommends,
quite reasonably, that you check the attribute first.  There is no
"RECOMMENDATION",
but most clients would prefer to do the simpler thing regardless.

I don't see the point of deleting this.  It simplifies life for the client,
and the burden on the
server side is trivial.

---

From: Spencer Dawkins at IETF <[email-masked]>
Date: Fri, 28 Apr 2017 18:24:18 -0500

Hi, David and Bruce,

On Fri, Apr 28, 2017 at 4:54 PM, David Noveck <[email-masked]> wrote:

> I don't see the point of deleting this.  It simplifies life for the
> client, and the burden on the
> server side is trivial.
>

Thanks for satisfying my curiosity.

 I don't mean to question the decision for either draft (-umask is already
in Last Call, and -xaddrs will be in Last Call Really Soon, so you can tell
I don't think the difference is a problem), but it made sense to me that I
should ask, in case the authors on either draft wanted to rethink the
decision.

Spencer (D), who does a lot of pattern matching when reviewing drafts that
he's less familiar with ;-)

---

From: [email-masked] (J. Bruce Fields)
Date: Sat, 29 Apr 2017 20:56:59 -0400

> a revision that defines ACE on first use after Last Call, that would be

OK, in practice I think the intended audience will have to be fairly
faimilar the terminology here, so I just added a hint, but hopefully
that's useful.

--b.

   <t>
     On many existing installations, all ordinary users by default use
     the same effective group ID.  To prevent granting all users full
-    access to each other's files, such installations usually default
-    to a umask with very restrictive permissions.  As a result,
-    inherited ACEs describing the permissions to be granted to named
-    users and groups are often ignored.  This makes inheritable ACLs
-    useless in some common cases.
+    access to each other's files, such installations usually default to
+    a umask with very restrictive permissions.  As a result, inherited
+    ACL entries (inheritable ACEs) describing the permissions to be
+    granted to named users and groups are often ignored.  This makes
+    inheritable ACEs useless in some common cases.
   </t>
 
   <t>