On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
> Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc ops are called
> by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all in control of
> vfs.
>
> nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so we put
> posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
> sattr->ia_mode value because vfs has did umask strip.
>
> Signed-off-by: Yang Xu <[email protected]>
> ---
I have the same comment as on the xfs patch. If the filesystem has opted
out of acls and SB_POSIXACL isn't set in sb->s_flags then
posix_acl_create() is a nop. Why bother placing it under an ifdef?
It adds visual noise and it implies that posix_acl_create() actually
does something even if the filesystem doesn't support posix acls.
Unless this actually fixes something I'd drop this patch too.
On Tue, 2022-04-19 at 15:59 +0200, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
> > Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc
> > ops are called
> > by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all
> > in control of
> > vfs.
> >
> > nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so
> > we put
> > posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
> > sattr->ia_mode value because vfs has did umask strip.
> >
> > Signed-off-by: Yang Xu <[email protected]>
> > ---
>
> I have the same comment as on the xfs patch. If the filesystem has
> opted
> out of acls and SB_POSIXACL isn't set in sb->s_flags then
> posix_acl_create() is a nop. Why bother placing it under an ifdef?
>
> It adds visual noise and it implies that posix_acl_create() actually
> does something even if the filesystem doesn't support posix acls.
>
Agreed and NACKed...
Any patch that gratuitously adds #ifdefs in situations where cleaner
alternatives exist is not going going to be applied by the NFS
maintainers.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
on 2022/4/19 22:11, Trond Myklebust wrote:
> On Tue, 2022-04-19 at 15:59 +0200, Christian Brauner wrote:
>> On Tue, Apr 19, 2022 at 07:47:10PM +0800, Yang Xu wrote:
>>> Since nfs3_proc_create/nfs3_proc_mkdir/nfs3_proc_mknod these rpc
>>> ops are called
>>> by nfs_create/nfs_mkdir/nfs_mkdir these inode ops, so they are all
>>> in control of
>>> vfs.
>>>
>>> nfs3_proc_setacls does nothing in the !CONFIG_NFS_V3_ACL case, so
>>> we put
>>> posix_acl_create under CONFIG_NFS_V3_ACL and it also doesn't affect
>>> sattr->ia_mode value because vfs has did umask strip.
>>>
>>> Signed-off-by: Yang Xu<[email protected]>
>>> ---
>>
>> I have the same comment as on the xfs patch. If the filesystem has
>> opted
>> out of acls and SB_POSIXACL isn't set in sb->s_flags then
>> posix_acl_create() is a nop. Why bother placing it under an ifdef?
>>
>> It adds visual noise and it implies that posix_acl_create() actually
>> does something even if the filesystem doesn't support posix acls.
>>
>
> Agreed and NACKed...
>
> Any patch that gratuitously adds #ifdefs in situations where cleaner
> alternatives exist is not going going to be applied by the NFS
> maintainers.
Ok, will drop this patch.
>