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

[Tom Haynes <[e-mail]>]

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 <[e-mail]> wrote:
> 
> 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/

fixed


> 
> 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.
> 


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
> 
> 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

“internal locks” -> “internal state”

Otherwise, taken verbatim


> 
> 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
> 

taken


> 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.

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?


> 
> 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.

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.