Subject: [Gen-art] Minor nits in minorversion2-40

From: Elwyn Davies <[email-masked]>
Date: Thu, 7 Jan 2016 14:20:44 +0000

Hi.

As suggested I downloaded your repository and made -40.

I had a quick look through and it is looking good.

Still to do/think about:
- 'we' removal
- structured privilege description expansion as per email.
- [BL73] - not reffed anymore.

I spotted a few items that appeared (or I had missed) - noted below.

Cheers,
Elwyn

Minor nits:
s4.2, para 2: s/intra-sever/intra-server/

s4.2.2, para 1: This sentence is a bit garbled:
> Other operations are OPTIONAL in the context of a particular feature 
> Section 13, but may become REQUIRED depending on server behavior.

s4.9, last para:
I was supposed to be letting you know if some extra explanation of why 
seqid being zero is ambiguous.... so, yes, I do think a bit extra is 
needed. Here goes:

s15.8.3 notes that there can be multiple file copies associated with a 
single file going on at the same time.  This is only implicit up to that 
point I think.  It would be helpful to add a note about this possibility 
and the availability of asynchronous copy in general to the intro of 
section 4.

BTW: removing the s4.1 header would be in keeping with usual style as 
you have already done for other sections.

BTW2:  I just realized that there is no general terminology section in 
this document.  Clearly most of it is taken over from either or both of 
RFC 7530 (s1.5) and RFC 5661 (s1.6).  What triggered this was the point 
that stateid isn't actually defined in this doc.  A reference to one or 
both of these and/or possibly some copies of definitions would be helpful.

In the following I may not have exactly grokked what the copy offload 
stateid represents... if so please adjust the words

Add to intro (was in s4.1, s/b in s4) as new last para:
ADD:
The copy feature allows the server to perform the copying either 
synchronously or asynchronously.  The client can request synchronous 
copying but the server may not be able to honor this request.  If the 
server intends to perform asynchronous copying, it supplies the client 
with a request identifier that  the client can use to monitor the 
progress of the copying and, if appropriate, cancel a request in 
progress.   The request identifier is a stateid representing the 
internal locks held by the server while the copying is performed. 
Multiple asynchronous copies of all or part of a file may be in progress 
in parallel on a server; the stateid request identifier allows 
monitoring and canceling to be applied to the correct request.
END

Then modify the last para of s4.9:
OLD:
    A copy offload stateid's seqid MUST NOT be zero.  In the context of a
    copy offload operation, it is ambiguous to indicate the most recent
    copy offload operation using a stateid with seqid of zero. Therefore
    a copy offload stateid with seqid of zero MUST be considered invalid.
NEW:
    A copy offload stateid's seqid MUST NOT be zero.  In the context of a
    copy offload operation, it is inappropriate to indicate "the most recent
    copy offload operation" using a stateid with seqid of zero (see 
Section 8.2.2
    of [RFC5661] for the meaning of a seqid of zero).  It is inappropriate
    because the stateid refers to internal state in the server and there 
may
    be several asynchronous copy operations being performed in parallel
    on the same file by the server.  Therefore
    a copy offload stateid with seqid of zero MUST be considered invalid.
END

s4.10, last para:
OLD:
    If a server requires the use of RPCSEC_GSSv3 copy_to_auth,
    copy_from_auth, or copy_confirm_auth and it is not used, the server
    will reject the request with NFS4ERR_PARTNER_NO_AUTH.

NEW:
    If a server requires the use of an RPCSEC_GSSv3 copy_to_auth,
    copy_from_auth, or copy_confirm_auth privilege and it is not used, 
the server
    will reject the request with NFS4ERR_PARTNER_NO_AUTH.

s4.10.1.1.1:  I understood you were going to say something about size 
and non-reuse of the random number?

s4.10.1.1.3, bullet 6: s/the COPY will be rejeced/the COPY will be rejected/

s8:  Did you think about 64bit big-endian/little-endian issues?

s9.2, last para: Need to expand LFS on first use. (missed in -39)

s10.5.6, para 2: (Not a change - I missed this in -39)
> Any file's layout obtained from a NFSv4.1 metadata server MUST NOT 
> have NFL42_UFLG_IO_ADVISE_THRU_MDS set.
I don't understand this statement.  If the layout is originated by an 
NFSv4.1 server, then I would interpret having this bit set as a server bug.

s12.1: One of the ADs complained about the weasel words in the Id column 
definition... the slightly less weaselly words from s5.6 of RFC7530 
should cure this.

s19.2:  Need to sort out [BL73].

---

From: Tom Haynes <[email-masked]>
Date: Fri, 22 Jan 2016 14:34:39 -0800

Hi Elwyn,

Playing catch-up after a big push in my day job.

Please understand this is not normal for me. :-)

I will be working my way forwards from here through the other emails and I am not peeking ahead.

Hi Andy,

I have one potential change below I’d like you to verify. It is the only other occurrence of “Andy"

> On Jan 7, 2016, at 6:20 AM, Elwyn Davies <[email-masked]> wrote:
> 
> Hi.
> 

fixed

>> Other operations are OPTIONAL in the context of a particular feature Section 13, but may become REQUIRED depending on server behavior.
> 

fixed

> s4.9, last para:
> I was supposed to be letting you know if some extra explanation of why seqid being zero is ambiguous.... so, yes, I do think a bit extra is needed. Here goes:
> 
> s15.8.3 notes that there can be multiple file copies associated with a single file going on at the same time.  This is only implicit up to that point I think.  It would be helpful to add a note about this possibility and the availability of asynchronous copy in general to the intro of section 4.

The async portion is covered by your later change below.

And you know what, I believe it also covers the part about multiple copies. :-)

> 
> BTW: removing the s4.1 header would be in keeping with usual style as you have already done for other sections.

fixed

> 
> BTW2:  I just realized that there is no general terminology section in this document.  Clearly most of it is taken over from either or both of RFC 7530 (s1.5) and RFC 5661 (s1.6).  What triggered this was the point that stateid isn't actually defined in this doc.  A reference to one or both of these and/or possibly some copies of definitions would be helpful.
> 
> In the following I may not have exactly grokked what the copy offload stateid represents... if so please adjust the words
> 
> The copy feature allows the server to perform the copying either synchronously or asynchronously.  The client can request synchronous copying but the server may not be able to honor this request.  If the server intends to perform asynchronous copying, it supplies the client with a request identifier that  the client can use to monitor the progress of the copying and, if appropriate, cancel a request in progress.   The request identifier is a stateid representing the internal locks held by the server while the copying is performed. Multiple asynchronous copies of all or part of a file may be in progress in parallel on a server; the stateid request identifier allows monitoring and canceling to be applied to the correct request.
> END

“internal locks” -> “internal state”

Otherwise, taken verbatim

>   copy offload operation" using a stateid with seqid of zero (see Section 8.2.2
>   because the stateid refers to internal state in the server and there may

taken

>   copy_from_auth, or copy_confirm_auth privilege and it is not used, the server

Okay, felt like a game of spot the ball in the newspaper, but done.

> 
> s4.10.1.1.1:  I understood you were going to say something about size and non-reuse of the random number?

More weasely words!

Andy,  I know nothing of rpcsec_gssv3. Are there constraints already in place for the random number?

                A random number is generated to use as a secret to be shared
                between the two servers.  This shared secret will be placed
                in the cfap_shared_secret and ctap_shared_secret fields of
                the appropriate privilege data types, copy_from_auth_priv
                and copy_to_auth_priv.  Because of this shared_secret the
                RPCSEC_GSS3_CREATE control messages for copy_from_auth
                and copy_to_auth MUST use a Quality of Protection (QOP) of rpc_gss_svc_privacy.

> s4.10.1.1.3, bullet 6: s/the COPY will be rejeced/the COPY will be rejected/

Done

> s8:  Did you think about 64bit big-endian/little-endian issues?

Point 2:

   2.  Fields to describe the state of the ADB and a means to detect
       block corruption.  For both pieces of data, a useful property
       would be that the allowed values are specially selected so that
       if passed across the network, corruption due to translation
       between big and little endian architectures is detectable.  For
       example, 0xF0DEDEF0 has the same (32 wide) bit pattern in both
       architectures, making it inappropriate.

> s9.2, last para: Need to expand LFS on first use. (missed in -39)

First use is in Section 9.1, right?

>> Any file's layout obtained from a NFSv4.1 metadata server MUST NOT have NFL42_UFLG_IO_ADVISE_THRU_MDS set.
> I don't understand this statement.  If the layout is originated by an NFSv4.1 server, then I would interpret having this bit set as a server bug.

Yes, hence the MUST - not trying to be flip here - this is the way in the protocol we specify a bug in the implementation.

> 
> s12.1: One of the ADs complained about the weasel words in the Id column definition... the slightly less weaselly words from s5.6 of RFC7530 should cure this.

I’ll push on this for now and check back at the end of all of the emails.

> s19.2:  Need to sort out [BL73].

Same here.

---

From: Tom Haynes <[email-masked]>
Date: Fri, 22 Jan 2016 16:17:13 -0800

Hi Elwyn,

After looking at the other responses, I’ve update the random number text in what
will be Section 4.9.1.1.1.

   o  A random number is generated to use as a secret to be shared
      between the two servers.  Note that the random number SHOULD not
      be reused between establishing different security contexts.  The
      resulting shared secret will be placed in the cap_shared_secret

I think [BL73] is the outstanding issue?

Thanks,
Tom

> On Jan 22, 2016, at 2:34 PM, Tom Haynes <[email-masked]> wrote:
> 
> Hi Elwyn,
>