Subject: [nfsv4] feedback on draft-ietf-nfsv4-versioning-07

From: Mike Kupfer <[email-masked]>
Date: Tue, 25 Oct 2016 13:54:47 -0700

Potential technical issues:

* Section 5.1, first bullet item: this bullet item seems to imply that
  any string-related changes must be done in a minor version, rather
  than an extension.  That's fine for the broad rules (e.g., i18n) that
  this bullet item seems to have in mind, but I think we should be able
  to extend a list of known literal strings in an extension.  We don't
  typically bake literal strings into the NFS specification, but we do
  use them occasionally (e.g., "nobody", as well as the special ACE
  identifiers like "OWNER@").

  (Sorry about just getting this out now--I missed it in previous
  reviews of my notes.)

* Section 9.2: after thinking about this some more, I'm still unsure on
  how things will work if the client implements only the uncorrected
  version and the server implements only the corrected version.  The
  client asks if the server supports minor version X and the server says
  yes.  At this point the client could ask about a correction to feature
  FOO, but it doesn't know about any such correction.  Since the client
  doesn't ask, the server might then be able to deduce that the client
  only supports the uncorrected version, but now it's too late--the
  server has already said it supports minor version X.  The first time
  the client will find out about the incompatibility is when it tries to
  use the uncorrected version of the feature and gets an error back from
  the server.

  It seems like the server must support both the uncorrected and
  corrected versions of the feature.  It can only drop support for the
  uncorrected version with the next minor version.

Minor editorial stuff:

* Section 1, penultimate paragraph: the current wording seems to imply
  that the XDR extensions approach will be able to handle all protocol
  corrections.  I would change "to make other sorts of changes" to "when
  needed".

* Section 1, last paragraph: change "is provide" to "is to provide"

* Page 5, last bullet item: change "subsequently" to "up to that time"

* Page 6, lines 4-5: change "it is the set is" to "it is"

* Page 6, last paragraph of Section 2.3: the last sentence of that
  paragraph bothers me, as it just echoes the first sentence of the
  previous paragraph.  Maybe replace the first sentence of the previous
  paragraph with 

    Each client and server which implements a specific minor version
    will implement some particular variant of that minor version.  Each
    variant is a subset of the current minor version and a superset of
    the base minor version.

  and drop the last paragraph completely?

* Page 7, third bullet item, sentence starting with "In particular": add
  a comma after "all previously specified minor versions".

* Page 9, second bullet item: change "operation codes" to "procedures".

* Page 9, third bullet item: change "RPC operations" to "RPC procedures,
  operation codes".

* Section 4.3, penultimate paragraph: remove the comma after "protocol
  elements".

* Page 11, second bullet item: change "subsequent minor version" to
  "subsequent minor versions" (add "s")

* Page 11, paragraph starting with "For many minor versions": change the
  first sentence to

    For many minor versions, all existing protocol elements are required
    to be known by both the client and the server, and so requesters do
    not have to test for the presence or absence of knowledge regarding
    optional protocol elements.

* Page 13, bullet item at the top of the page, sentence starting with
  "The client can determine which of the extensions": change to

    The client can use the approach described in Section 4.4.3 to
    determine which of the extensions it knows about are also known by
    the server.

* Section 4.5, first paragraph: change "fields." to "fields that are"

* Section 5, first paragraph: add a period at the end of the paragraph.

* Page 16, last bullet item: change "indicated non-support a flag bit"
  to "indicating non-support of a flag bit".

* Page 17, last paragraph: "OPTONAL" -> "OPTIONAL"

* Section 9, first paragraph: add a comma after "acceptance of that feature"

* Section 9, 2nd paragraph, first sentence: 

    Such corrections are best done in a document obsoleting or updating
    the RFC defining the relevant feature definition document or minor
    version specification.

  I think there are too many words here.  We could refer to "the RFC
  defining the relevant feature or minor version".  Or we could refer to
  the "relevant feature definition document or minor version
  specification".

* Section 9.1, 2nd bullet item: change "appropriate" to "appropriate way"

* Page 20, 2nd bullet item: change "feature, be considered obsolescent"
  to "feature be considered obsolescent"

* Page 20, paragraph starting with "By doing things this way": I would
  drop the bullet list that follows this paragraph and replace the
  paragraph with

    By doing things this way, a requester can determine whether the
    other host supports a particular version of the feature.  As a
    result, the protocol with the XDR modification can accommodate
    clients and servers that support either the corrected or the
    uncorrected version of the protocol, as well as clients and servers
    that support both alternatives.

* Section 9.2 title: should "required" be all uppercase?

Okay, that's everything I have flagged in my notes.

mike

---

From: David Noveck <[email-masked]>
Date: Wed, 26 Oct 2016 09:10:47 -0400

> Potential technical issues:

I don't think of either of these as difficult to resolve.

I'll discuss each one and then we can consider the possible effect on WGLC,
given
the onrushing 10/31 (!) document submission witching hour.

> but I think we should be able to extend a list of known literal
> strings in an extension.

I can see why you might want that, but it is difficult to provide for.
When you are testing for support or knowledge of an XDR protocol element,
the draft  already has a description of how to do the testing, the errors
that might result, etc.

If you are going to allow those that do or do not support a extension with
regard to the strings accepted, you have to provide analogous means to find
out what is supported.  I fear that changing the document at this point to
provide for that would be difficult to do, especially in a case where
strings are added to attribute values.  In that case you need to know
whether the client will accept them on GETATTR as well as whether the
server will accept them on SETATTR.

If you want to have variant/extended handling of the set of strings, you
could define a new attribute that allowed the new strings together with
retaining  an older one that accepted only the old strings.  I think this
would give you what you need, leaving the extension framework as it is.

> At this point the client could ask about a correction to feature
> FOO, but it doesn't know about any such correction.

Good point.  The difference from the rest of the extension model is that
the old client, having been written before the correction was identified,
has no knowledge of the potential non-support and thus cannot test for it
in advance.

> corrected versions of the feature.

Lets call the version subject to correction 4.x.

4.x requires the uncorrected version and and cannot require the corrected
version.

I think it can allow (not require) the corrected version.

>It can only drop support for the uncorrected version with the
> next minor version.

The working group would have a number of options in 4.x+1:

Making the corrected version REQUIRED and the uncorrected version mandatory
to not implement.  This would force older clients to use and servers to
support the corrected version. befire adopting 4.x+1

Making the corrected version REQUIRED and the uncorrected version OPTIONAL
 This would force server to support the corrected version but 4.x clients
could have a reprieve and could simply send the 4.x request as 4.x+1, with
no guarantee of support.  However, it would test for support of the
uncorrected version as an OPTIONAL feature.

Making both the corrected and uncorrected version OPTIONAL with the
constraint that at least one has to be supported.  This would allow both
uncorrected clients and servers to be easily converted to 4.x+1,  However,
there would be no guarantee of interoperability for servers or clents only
prepared to deal with the corrected or corrected variant.

With regard to WGLC, we have two options:

Consider your comments an early contribution to the about-to-be-announced
WGLC of -07

Wait until the submission hiatus to submit a -08 and make that the subject
of WGLC.  That document could respond to the technical issues and address
the editorial issues you raised in your mail

I'm OK with either of the above but feel that the WGLC for umask-02 and
xattrs-03 should go forward now in any case.

On Tue, Oct 25, 2016 at 4:54 PM, Mike Kupfer <[email-masked]> wrote:

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

---

From: Mike Kupfer <[email-masked]>
Date: Wed, 26 Oct 2016 15:34:19 -0700

>> but I think we should be able to extend a list of known literal
[...]
> out what is supported.

Hmm.  I was thinking that adding a new string would be similar to adding
a new value to an enum.  But I now see that although Section 4.2 says
that an extension can add values to enums, the document doesn't say how
that would be managed.

> I fear that changing the document at this point to

Yes, the server would need to know that the client supports a particular
extension before returning values that are defined by that extension.

Adding a new attribute or operation that knows about the extended type
sounds reasonable to me.  That approach could be used for both new
strings and new values in an enum.

So it sounds like this is just an editorial issue, to clarify how new
enum values or string literals could be added by an extension.

Here's a suggestion for text to add at the end of Section 4.2:

  If an enum may be contained in an RPC response, an extension that adds
  one or more new values to the enum must additionally provide a way for
  the responder to determine that the requester knows about the
  extension.  For example, an extension that defines a new file type
  could define new operations to use instead of GETATTR and SETATTR.
  The GETATTR and SETATTR operations would continue to support just the
  original set of file types.  The new operations would support the
  original file types as well as the new one.

I'd still like to see the ability to extend a list of well-known strings
without changing the minor version.  I think that adding the following
text to the first bullet item in Section 5.1 would cover it:

  In some cases, an extension may add new string literals to a set of
  well-known strings, similar to the way that new values may be added to
  an enum.  Consideration must be given to whether the string might
  already be in use.  And as with enums, there are additional
  requirements if the string could be included in an RPC response (see
  Section 4.2 for details).

> 4.x requires the uncorrected version and and cannot require the corrected

Right.  I meant that *if* the server supports the corrected version, it
must still support the uncorrected version.

> uncorrected version as an OPTIONAL feature.

And if the uncorrected version is not supported, the client would fall
back to 4.x.

> prepared to deal with the corrected or corrected variant.

Yeah.  So I think we'd want to discourage that last choice.

> With regard to WGLC, we have two options:
[...]
I have no opinion on WGLC for any of the I-Ds.  But then I'm not into
country music[1]. :-)

cheers,
mike

[1] http://www.wglc.net

---

From: David Noveck <[email-masked]>
Date: Thu, 27 Oct 2016 06:01:22 -0400

I think we are close to agreement regarding how to address the issues in
section 9.

Regarding strings/enums, I don't think we want to try to expand the scope
of what can be done in an extension.  The -05 draft allowed knowledge of an
additional XDR element as part of an extension to signal whether a field
use change such as one to the set of valid values of a string was present.
However, the working group found that too complicated and in -06 I focused
 on the simplest extension model, in which each of the kinds of changes
allowed by the original minor versioning approach was allowed in an
extension and other things required a minor version.  That might not be the
most flexible approach but it is way better than where we are now, with
every change requiring a minor version.

Regarding extending enums, it is true that -07 seems to allow this without
saying how to mange it in general.  It  only discusses expanding switches
and provides specific rules regarding error codes, operation codes, and
attribute numbers.

The real problem with allowing general enum extensions is that XDR
implementations do not check that the values in the fields are valid.  As a
result, you cannot test that the responder knows about the new values by
issuing a request containing the new value, as you can with extensions to
switches.

As a result, enum fields are essentially ints, and the restriction to the
values listed in the XDR file is much like an in-spec-text restriction
regarding the contents of that particular field. I think the spec should
make clear it that changing the set of allowed values requires a new minor
version.

> But then I'm not into country music[1]. :-)

Neither am I.

---

From: [email-masked] (J. Bruce Fields)
Date: Thu, 27 Oct 2016 14:14:11 -0400

On Thu, Oct 27, 2016 at 06:01:22AM -0400, David Noveck wrote:
> implementations do not check that the values in the fields are valid.

It may not be caught at xdr decoding time, but in most cases some other
part of the code will have to catch it.  E.g. 5661 18.16.4 requires
implementations to return INVAL on unknown share_access values.

(IO_ADVISE is an interesting case--the server's required to ignore
unsupported values.  That might not extend to undefined values.  But as
it's only a hint, clients could probably deal with either behavior
easily enough.)

The important principal here that we preserve interoperability with
existing implementations.  Applying that principal to specific protocol
exentensions is going to be more useful than trying to enumerate general
rules about extending different kinds of protocol elements.

--b.

> As a

---

From: Mike Kupfer <[email-masked]>
Date: Thu, 27 Oct 2016 11:22:26 -0700

> issuing a request containing the new value, as you can with extensions to

Well, there won't be a check at the RPC layer.  But presumably the NFS
layer would return something like NFS4ERR_INVAL if it's given a value it
doesn't know about.

> make clear it that changing the set of allowed values requires a new minor

I'm okay with narrowing the scope of what an extension can do.  But if
we require a minor version bump to change the set of allowed values for
an enum, we should do the same for the general case of flag words.  (New
attributes could still be added via an extension.)

mike

---

From: David Noveck <[email-masked]>
Date: Fri, 28 Oct 2016 08:29:26 -0400

l've looked at how bit flags are dealt with in -07 and believe that enum
extensions can be dealt with similarly.
They are both constructs where  the generated XDR code does not check for
validity and thus they raise similar issues.

In reviewing the treatment, I have found  few gap in the treatment of bit
flags.  Those need to be fixed and I expect that the handling of enum
extensions will be parallel:

   - There is no discussion of adding bit flags in existing responses.
   Instead, it should be made explicit that doing this would require a new
   minor version.
   - There is no explicit discussion of the fact that, for these additions,
   the interest cannot be distinguish between an unknown bit flag and one
   which is known but unsupported.  This can have implication for how the
   client and server decide on a common XDR variant.  I'd hope that this issue
   can treated in a way that allows a future minor version to better address
   the issue by  reorganizing error codes.  My preference would be for
   preservng NFS4ERR_INVAL as indicating an unknown bit/enum, while allowing
   future versions to address unsupported stuff in a way analogous that Tom
   did for switches in v4.2, i.e. by creating NFS4ERR_FLAGBIT_NOTSUPP and
   NFS4ERR_ENUMVAL_NOTSUPP.

---

From: Mike Kupfer <[email-masked]>
Date: Fri, 28 Oct 2016 14:37:49 -0700

> extensions can be dealt with similarly.

Okay.

>    Instead, it should be made explicit that doing this would require a new

Okay.

>    did for switches in v4.2, i.e. by creating NFS4ERR_FLAGBIT_NOTSUPP and

Adding new "notsupp" error values sounds reasonable to me.

I'm less enthusiastic about using NFS4ERR_INVAL to mean either "unknown
enum/flag" or "understood but invalid request".  We discussed this back
in June, and I accept the argument for minor version 2 that allowing
either meaning matches current server behavior.  And it's probably
manageable as long as sufficient attention is given in an extension
specification to how the requester determines that the extension is
supported.  But in the long run, I still think it would be cleaner to
split those two meanings for NFS4ERR_INVAL into separate error codes.

mike

---

From: [email-masked] (J. Bruce Fields)
Date: Sun, 30 Oct 2016 22:04:35 -0400

>    Instead, it should be made explicit that doing this would require a new

So, that rules out adding attributes?  (Since you'd have to return new
bits in the supported_attribute attribute).

--b.

---

From: David Noveck <[email-masked]>
Date: Mon, 31 Oct 2016 13:25:39 -0400

> So, that rules out adding attributes?

No it doesn't.

> (Since you'd have to return new bits in the supported_attribute
attribute).

I said adding "bit flags" and I think that means a new definition of a
previously unknown flag bit.

Each bit in the bit mask of supported attribute already has a definition
and when I write the text, I'll make sure that it is clear that expanding
supported_attrs is not going to change that  For example bit 128 means the
server supports attribute 128.  The further information regarding what
attribute that is would be provided by the extension that defines attribute
128, the effect of which is already clear.  The point is that the client
who does not know about attribute 128 is not going to puzzle over this new
bit.  He knows it indicates that the server supports some attribute he
doesn't know about.

---

From: Trond Myklebust <[email-masked]>
Date: Mon, 31 Oct 2016 13:57:40 -0400

> On Oct 31, 2016, at 13:25, David Noveck <[email-masked]> wrote:
> 
> > (Since you'd have to return new bits in the supported_attribute attribute).
> 
> I said adding "bit flags" and I think that means a new definition of a previously unknown flag bit.
> 
> Each bit in the bit mask of supported attribute already has a definition and when I write the text, I'll make sure that it is clear that expanding supported_attrs is not going to change that  For example bit 128 means the server supports attribute 128.  The further information regarding what attribute that is would be provided by the extension that defines attribute 128, the effect of which is already clear.  The point is that the client who does not know about attribute 128 is not going to puzzle over this new bit.  He knows it indicates that the server supports some attribute he doesn't know about.

One thing to note is that we’ve had issues before where both clients and servers have been known to assume things about the maximum size of the bitmaps being returned.

> 
> On Sun, Oct 30, 2016 at 10:04 PM, J. Bruce Fields <[email-masked] <mailto:[email-masked]>> wrote:
> > On Thu, Oct 27, 2016 at 2:22 PM, Mike Kupfer <[email-masked] <mailto:[email-masked]>> wrote:
> >
> > > David Noveck <[email-masked] <mailto:[email-masked]>> writes:
> > >
> > [email-masked] <mailto:[email-masked]>
> > https://www.ietf.org/mailman/listinfo/nfsv4 <https://www.ietf.org/mailman/listinfo/nfsv4>
> 
>

---

From: Fields Bruce James <[email-masked]>
Date: Mon, 31 Oct 2016 17:08:04 -0400

On Mon, Oct 31, 2016 at 01:57:40PM -0400, Trond Myklebust wrote:
> > Each bit in the bit mask of supported attribute already has a
> > definition and when I write the text, I'll make sure that it is
> > clear that expanding supported_attrs is not going to change that
> > For example bit 128 means the server supports attribute 128.  The
> > further information regarding what attribute that is would be
> > provided by the extension that defines attribute 128, the effect of
> > which is already clear.  The point is that the client who does not
> > know about attribute 128 is not going to puzzle over this new bit.
> > He knows it indicates that the server supports some attribute he
> > doesn't know about.

That's a subtle distinction.  I'm not sure I completely understand the
rationale.  So adding a new attribute flag is OK, but e.g. a new open
result flag is out?  Are you judging that a client is more likely to
give up and throw an EIO on an unknown open result flag than on an
unknown attribute flag?

Personally I'd put my money on the attribute case being more likely, if
only because it's variable-length and, as Trond says, we've actually
seen bugs here before in practice.

> One thing to note is that we’ve had issues before where both clients
> and servers have been known to assume things about the maximum size of
> the bitmaps being returned.

I wasn't too worried about this kind of thing when I thought we were
just talking about allowing new minorversions to be extensible, or maybe
opening up 4.2 in retrospect, but extending 4.0 this way makes me
nervous.

Bugs happen, and we fix them, but the older the protocol the more likely
there's old implementations out there that aren't getting much
maintenance.  I don't know.

--b.