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

Chuck Lever
2020-03-12T17:57:32Z

> 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