Re: [PATCH 06/14] nfsd: define xattr functions to call in to their vfs counterparts

From: Chuck Lever <chuck.lever@oracle.com>
To: Frank van der Linden <fllinden@amazon.com>
Cc: Bruce Fields <bfields@fieldses.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 06/14] nfsd: define xattr functions to call in to their vfs counterparts
Date: Thu, 12 Mar 2020 13:57:25 -0400	[thread overview]
Message-ID: <D8888578-85D0-4824-9658-F5DAF1F27026@oracle.com> (raw)
In-Reply-To: <20200312171630.GA11733@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com>

> On Mar 12, 2020, at 1:16 PM, Frank van der Linden <fllinden@amazon.com> wrote:
> 
> On Thu, Mar 12, 2020 at 12:23:57PM -0400, Chuck Lever wrote:
>>> On Mar 11, 2020, at 3:59 PM, Frank van der Linden <fllinden@amazon.com> wrote:
>>> 
>>> This adds the filehandle based functions for the xattr operations
>>> that call in to the vfs layer to do the actual work.
>> 
>> Before posting again, use "make C=1" to clear up new sparse
>> errors, and scripts/checkpatch.pl to find and correct whitespace
>> damage introduced in this series.
> 
> Hi Chuck,
> 
> Thanks for this comment (and the other ones).
> 
> I forgot to run sparse - I have fixed the __be32/int type mismatches
> it flagged in my tree.
> 
> I ran checkpath.pl before sending these in. Looks like I missed
> one line that is too long. The warnings I didn't fix were:
> 
> ==
> WARNING: function definition argument 'struct dentry *' should also have an identifier name
> #156: FILE: include/linux/xattr.h:54:
> +int __vfs_setxattr_locked(struct dentry *, const char *, const void *, size_t, int, struct inode **);
> ==
> 
> ..changing this would make the prototype declaration of that function not
> match with the style of the ones around it (which also don't have argument
> names in their declarations) - so I decided not to.
> 
> The other one is:
> 
> ===
> WARNING: please, no spaces at the start of a line
> #46: FILE: fs/nfsd/vfs.c:616:
> +    {^INFS4_ACCESS_XAREAD,^INFSD_MAY_READ^I^I^I},$
> ===
> 
> The warning is correct, but the array that is part of, and the surrounding
> accessmap arrays, all have the same spacing. So to both silence checkpacth
> and make the code look consistent, I'd have to change the spacing in
> all those arrays (nfs3_regaccess, nfs3_diraccess, nfs3_anyaccess).
> 
> This didn't seem right, so I didn't do it.

Fair enough, please add a comment to that effect to the patch description.

> I'll be happy to send a separate whitespace cleanup for that, not sure
> if it should be part of this series, though.
> 
> Frank

--
Chuck Lever