Subject: [Gen-art] Gen-art LC review of draft-ietf-nfsv4-rfc3530bis-dot-x-22

From: Elwyn Davies <[email-masked]>
Date: Wed, 12 Nov 2014 22:27:23 +0000

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-nfsv4-rfc3530bis-dot-x-22.txt
Reviewer: Elwyn Davies
Review Date: 2014-11-12
IETF LC End Date: 2014-10-06
IESG Telechat date: 2014-12-04

Summary:
The main aims of this review were to
1. Check that the extracted .x file actually compiled (it didn't) - fix 
below.
2. To check the text corresponded with draft-ietf-nfsv4-rfc3530bis-33/34

I found a few minor glitches but otherwise this draft is almost ready.

Major Issues:
(well not really major)
The extracted .x file does not currently compile with rpcgen.
The cause is the definition of linktext4 (at line 154 of the extracted 
file) which was just fixed in draft-ietf-nfsv4-rfc3530bis-34 but not in 
this one.
s/linktext4/linktext4<>/

Minor Issues:

The type used for the lease_time attribute in -dot-x-22 is uint32_t, i.e.,
typedef uint32_t fattr4_lease_time;
In 3530bis the type shown in Table 2 is nfs_lease4 but there is no 
mapping from this type to a base integer type in either document.  This 
needs cleaning up.

========================

Differences between rfc3530-33/34 and rfc35230bus-dot-x-22:
-----------------------------------------------------------

The items below need fixing up in 3530bis (apart from possibly the 
lease_time type) to make them consistent with -dot-x-22.

- Error codes NFS4ERR_CB_PATH_DOWN and NFS4ERR_SHARE_DENIED are missing 
from table 5 in rfc3530-33 and there are no corresponding sections in 
Section 13.1.1 of rfc3530-33.  However, they are mentioned in the text 
and in Tables 6 and 8.  BTW All these tables ought to have titles 
(assuming they remain after Barry's comments).  Also the ordering in 
Table 5 is slightly out of dictionary sort order if anyone cares.

- Required attributes (Table 2 in rfc3530bis):
   + change attribute type s/b changeid4
   + lease_time attribute the type used in -dot-x is uint32_t -
     nfs_lease4 used in 3530bis-33 is not defined in dot-x or copied
     into 3530bis.
   + fs_locations attribute type s/b fs_locations4 (not fs_locations)
   + owner attribute type s/b utf8str_mixed (not utf8<>)
   + owner_group attribute type s/b utf8str_mixed (not utf8<>)

---

From: Tom Haynes <[email-masked]>
Date: Wed, 12 Nov 2014 15:18:32 -0800

Hi Elwyn,

Thanks for examining across both documents.
Tom

> On Nov 12, 2014, at 2:27 PM, Elwyn Davies <[email-masked]> wrote:
> 
> 1. Check that the extracted .x file actually compiled (it didn't) - fix below.
> The cause is the definition of linktext4 (at line 154 of the extracted file) which was just fixed in draft-ietf-nfsv4-rfc3530bis-34 but not in this one.
> s/linktext4/linktext4<>/
> 
> 

I’m pretty sure that was how I fixed it in 34 in the first place. :-)

And checking what would be draft 23, it is fixed there as well.

> In 3530bis the type shown in Table 2 is nfs_lease4 but there is no mapping from this type to a base integer type in either document.  This needs cleaning up.

Done - thanks for the pointer to fattr4_lease_time- fixed as well now.

> The items below need fixing up in 3530bis (apart from possibly the lease_time type) to make them consistent with -dot-x-22.
> 
> - Error codes NFS4ERR_CB_PATH_DOWN and NFS4ERR_SHARE_DENIED are missing from table 5 in rfc3530-33 and there are no corresponding sections in Section 13.1.1 of rfc3530-33.

Done

>  However, they are mentioned in the text and in Tables 6 and 8.  BTW All these tables ought to have titles (assuming they remain after Barry's comments).  

So Table 8 needs a title. Done

> Also the ordering in Table 5 is slightly out of dictionary sort order if anyone cares.

Fixed NFS4ERR_ATTRNOTSUPP.

NFS4_OK is first by convention. Do you want it changed?

I.e., I do care a bit. :-)

>  + change attribute type s/b changeid4

Also in 5661

fixed

>    nfs_lease4 used in 3530bis-33 is not defined in dot-x or copied

fixed

>  + fs_locations attribute type s/b fs_locations4 (not fs_locations)

Hmm, also an issue in RFC 5661

fixed

>  + owner_group attribute type s/b utf8str_mixed (not utf8<>)

Both fixed