Re: [nfsv4] AD review: draft-ietf-nfsv4-delstid-03

> On Oct 5, 2023, at 4:42 AM, Zaheduzzaman Sarker <zahed.sarker.ietf@gmail.com> wrote:
> 

I’ll address the rest of the issues once I get some time, but I clarified what I meant in the XDR section.

> 
> 
> On Thu, Oct 5, 2023 at 1:16 PM David Noveck <davenoveck@gmail.com <mailto:davenoveck@gmail.com>> wrote:
> 
> 
> On Thu, Oct 5, 2023 at 6:43 AM Zaheduzzaman Sarker <zahed.sarker.ietf@gmail.com <mailto:zahed.sarker.ietf@gmail.com>> wrote:
> Hi all,
> 
> Now I have done my AD review of draft-ietf-nfsv4-delstid-03. I think this document needs some rewrites and clarifications before we can proceed further. See my comments below.
> 
> Comments :
> 
> # We need a section describing what are we updating in a nutshell. The way it is now written makes it is extremely difficult to understand what has been modified/amended/updated compared to RFC 8881.
> 
> Since this is an extension to NFSv4.2,, the comparison should be to  RFC7862/
> 
> The point here is to be clear about what we are doing.
>  
> 
> 
> # Please clearly mention that this specification will update RFC8881 in the abstract.
> 
> It does not update really RFC8881, even though the header says it does.  Although the title says "NFSv4.2", it does not need to update RFC7862 either.  Please see RFC 8178  regarding this issue.
> So this means this document should not really say it is updating any of those RFCs, or? 
>   
> 
> # Why are we defining delegation and stateid again here? What is the change in the definition that requires the redefinition ? Can't we use what is defined in RFC8881?
> 
> # Proxy definition : does this mean the client is proxying to itself? Can this proxy be somewhere in the network between the client and server. RFC8881 states server could also act as proxy hence this proxy definition needs to be clear about who can be proxy. I would also like to understand the difference between delegation and proxying.
> 
> # I would strongly suggest to define/describe "offline files". I have hard time understanding of a file being offline from whos perspective? Will a file be considered offline if the client for some reason cannot access the cloud storage or is this only about archived files?  
> 
> # Please explain or describe the code blocks, otherwise they seems to come out of nowhere and does not fit themselves well in the sections.
> 
> # I think we should gather all the new flags defined in one section or sub-section and describe them. This will also help to see what are we updating.
> 
> # As we are defining new procedure and new flag, how are we handling the errors (we not have number of MUSTs, RECOMMENDs and SHOULDs)?
> 
> # Section 3: 
>     - It says -
>          "If the client gets both a open and a delegation stateid as part of the OPEN, then it has to return them both."
>     Return to where?
>     - Where is OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION and OPEN4_RESULT_NO_OPEN_STATEID  delegation flags defined ?
> 
> # Section 3.1: 
>    - Put a reference to the "CLOSE operation".
>    - Where is OPEN_ARGS_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION flag defined?
>    - What is DELEGRETURN? yes, refer to RFC8881.
> 
> # Section 4:
>    - I am not sure I understand the statement - "While it is the authority for these values, it has no way to transfer these values to the server when the delegation is being returned and the server reclaims its authority with regard to these values."
>      I thought DELEGRETURN is a way to transfer or return those values. RFC 8881 says - "Delegations may be returned voluntarily (i.e., before the server has recalled them) or when recalled. In either case, the client must properly propagate state changed under the context of the delegation to the server before returning the delegation". Can we please explain it in a better way, like what values we are taking about and why they cant be transferred?.
>     - "These new attributes are invalid GETATTR, VERIFY, and NVERIFY" - there is something missing in this statement. "invalid to be used with" ?
>     - Please describe/explain/reference - "old times", "past the current time", and "original time". also put references for "access time", "change time", "modify time".
>     - Where is FATTR4_TIME_DELEG_ACCESS and FATTR4_TIME_DELEG_MODIFY defined?
>     - Give a ref to NFS4ERR_DELAY.
> 
> # Section 4.1 seems to me a good motivation for this extension, may be we should uplift if to a separate motivation section.
> 
> # Section 4.2: If we want someone to review this document, how would one read this section? please describe the code blocks, what they do or mean or related to the sections where these blocks are described.
> 
> # May be section 5 is misplaced, may be we should have it before section 3 and 4, so we define them and then show how the are used. the alternative would be to refer them from section 3 and section 4.
> 
> # Section 5: 
>     - It says - " client MUST determine that the additional new OPTIONAL functionality to OPEN is also not supported". what is the procedure for the client to determine that?
> 
> # Section 6: 
>     - It says - "While the XDR can be appended to that from [RFC7863 <https://www.ietf.org/archive/id/draft-ietf-nfsv4-delstid-03.html#RFC7863>], the various code snippets belong in their respective areas of the that XDR.". Does this mean this document also should update RFC7863?
> 
> I don't think it should do so.  That hasn't been the practice with other extensions to NFSv4.2. 
> 
> Then that referred statement need to be elaborated to reflect what it actually does.  Like could append but not update the original RFC7863 and refer to "the practice"
> 

The intent of the paragraph is that:

    While you could take the XDR generated from RFC7863 and append the new XDR to the end, what you should do is place the new XDR in the corresponding place inside that of RFC7863.

This is entirely a procedural process for people who maintain their own .x files. There is no notion that is updates or replace RFC7863.

> //Zahed
> 
> 
> 
> # Section 7: in this specification we are extending some capabilities for client delegation. I think we need more convincing description that there is not security concerns here.
> 
> //Zahed
> 
> 
>  
> 
> 
> 
> _______________________________________________
> nfsv4 mailing list
> nfsv4@ietf.org <mailto:nfsv4@ietf.org>
> https://www.ietf.org/mailman/listinfo/nfsv4 <https://www.ietf.org/mailman/listinfo/nfsv4>