[nfsv4] review of draft-ietf-nfsv4-internationalization-10.txt

[Christoph Hellwig <[e-mail]>]

Hi Dave,

this is a review of draft-ietf-nfsv4-internationalization-10.txt.

First, please run the document through a spell checker, there are a lot
of typos and annoying inconsistencies like using implementers and
implementors right next to each other.

Second, please refer to my recent mail to the WG list about covering 4.0
or not.  I think that needs to be a WG conclusion an is not document
specific, but highly relevant here.

The document refers to the term 'physical file systems' without
explaining it first.  A more common term would be 'local file system'
but even that is not entirely correct as NFS servers can also
export remote or distributed file systems, so 'underlying file system'
might be a better choice.  Either way the term should be explained in
the General Definitions section.

Section 2.2. "Requirements Language Derivation" is very confusing.
The BCP 14 terms designate requirements of the specification, and
the reason why the specification has these requirements does not
matter.  As far as I can tell simply removing this section would
improve the document significantly.

The forward reference from Section 6 to Section 7 harms the readability
of the document.  Instead of referring to different modes of operation
that are defined later, please define the high level operation modes
earlier my moving section 7 before section 6. In general I also don't
find references to "NFSv3" or "older" string handling very useful.
To an unencumbered implementer, spelling out the actual modes with
descriptive names will be a lot more useful and descriptive and useful.
That does not preclude mentioning what NFSv3 did, but it should be part
of the primary definition.  It would also really help to define the
two operation modes and the terms used by them in a General Definitions
section.

The name caching described in Section 6.3 can work in a slightly modified
form when file name normalization is used.  Linux implements it for
local file systems using Unicode based normalization and case
insensitivity.  In the NFS context that would require that the
client knows the exact normalization behavior and Unicode version used
on the server, and thus is relatively impractical.  Given how little
NFS is used with case insensitive file systems is also doesn't
really matter.

Section 8 and especially Section 8.1 are very meandering and the
wording feels like an essay and not suitable for a normative document.

I would suggest to replace Section 8 with a short introduction of
what changes compared to the definition in RFC8881 and drop section
8.1 entirely.  The details history of the text is of no importance to
implementers.  If you feel it should be properly archive somewhere
an informational document on the history of internationalization in
NFS might be somewhat interesting.

Section 8.2 should be worded as actual normative language.  My
quick and dirty proposal is below:

-------------------------------- snip --------------------------------
This section replaces Section 14.4 of [RFC8881], taking into account the
behavior of existing implementation of [RFC5661] [RFC8881] while
providing best effort compatibility with the definition in [RFC5661] and
[RFC8881].


  const FSCHARSET_CAP4_CONTAINS_NON_UTF8  = 0x1;
  const FSCHARSET_CAP4_ALLOWS_ONLY_UTF8   = 0x2;

  typedef uint32_t        fs_charset_cap4;

This attribute provides a simple way of determining whether a particular
file system behaves as a UTF-8-only server and rejects file names which
are not valid UTF8-encoded strings. When this attribute is supported and
the value returned has the FSCHARSET_CAP4_ALLOWS_ONLY_UTF8 flag set, the
error NFS4ERR_INVAL MUST be returned if any file name argument contains a
string which is not a valid UTF8-encoded string.

When this attribute is supported and the value returned has the
FSCHARSET_CAP4_ALLOWS_ONLY_UTF8 flag clear, the error NFS4ERR_INVAL will
not be returned based on adherence to the rules of UTF-8.

The FSCHARSET_CAP4_CONTAINS_NON_UTF8 flags exist for historical reasons
only and has no clear behavior associated with it.  Servers SHOULD set
the FSCHARSET_CAP4_CONTAINS_NON_UTF8 when they set the
FSCHARSET_CAP4_ALLOWS_ONLY_UTF8 flag, and clear the
FSCHARSET_CAP4_CONTAINS_NON_UTF8 when they clear
FSCHARSET_CAP4_ALLOWS_ONLY_UTF8 to provide the best compatibility to
historic clients.

Clients SHOULD ignore the FSCHARSET_CAP4_CONTAINS_NON_UTF8 flag.

When the fs_charset_cap attribute is not supported, the client can
perform a LOOKUP using a name not conforming to the rules of UTF-8
and use the error returned to determine whether non-UTF-8 names are
accepted.
-------------------------------- snip --------------------------------

Section 12 is closely related to Sections 6 and 7 and it would
be very helpful if it was located close to them in the document.

The section could use a bit of rewording, and the use of BCP 14
terms to describe actual server behavior and not just protocol
requirements seems wrong.  And possible replacement for the start
of this section is provided below:

-------------------------------- snip --------------------------------
Servers MAY accept component names that are not valid UTF-8 strings on
all or on some subset of file systems exported.

A typical pattern is for a server to use UTF‑8-unaware underlying file
systems that treat component names as uninterpreted strings of bytes,
rather than having any awareness of the character set being used.
Such servers do not change the stored representation of component names
from those received on the wire and use an octet-by-octet comparison of
component name strings to determine equivalence (as opposed to any
broader notion of string comparison).

This is because the server has no knowledge of the specific character
encoding being used.
-------------------------------- snip --------------------------------

Section 13 feels like editorializing.  I don't think this belongs into
a normative section.  If you feel attached to it maybe move it to
an appendix or the above mentioned informational document?

In general the document could use a bit less of term "As stated"
{,above,previously}".

This review didn't make it to the appendices yet, but I plan to
look over them in the future.