diff mbox series

[v4,4/8] NFSv3: only do posix_acl_create under CONFIG_NFS_V3_ACL

Message ID 1650368834-2420-4-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip | expand

Commit Message

Yang Xu (Fujitsu) April 19, 2022, 11:47 a.m. UTC
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 <xuyang2018.jy@fujitsu.com>
---
 fs/nfs/nfs3proc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Christian Brauner April 19, 2022, 1:59 p.m. UTC | #1
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 <xuyang2018.jy@fujitsu.com>
> ---

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.
Trond Myklebust April 19, 2022, 2:11 p.m. UTC | #2
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 <xuyang2018.jy@fujitsu.com>
> > ---
> 
> 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.
Yang Xu (Fujitsu) April 20, 2022, 1:12 a.m. UTC | #3
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<xuyang2018.jy@fujitsu.com>
>>> ---
>>
>> 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.
>
diff mbox series

Patch

diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index 1597eef40d54..9ab93427db30 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -337,7 +337,7 @@  static int
 nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		 int flags)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -361,9 +361,11 @@  nfs3_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		data->arg.create.verifier[1] = cpu_to_be32(current->pid);
 	}
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	for (;;) {
 		d_alias = nfs3_do_create(dir, dentry, data);
@@ -580,7 +582,7 @@  nfs3_proc_symlink(struct inode *dir, struct dentry *dentry, struct page *page,
 static int
 nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -591,9 +593,11 @@  nfs3_proc_mkdir(struct inode *dir, struct dentry *dentry, struct iattr *sattr)
 	if (data == NULL)
 		goto out;
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKDIR];
 	data->arg.mkdir.fh = NFS_FH(dir);
@@ -711,7 +715,7 @@  static int
 nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 		dev_t rdev)
 {
-	struct posix_acl *default_acl, *acl;
+	struct posix_acl *default_acl = NULL, *acl = NULL;
 	struct nfs3_createdata *data;
 	struct dentry *d_alias;
 	int status = -ENOMEM;
@@ -723,9 +727,11 @@  nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
 	if (data == NULL)
 		goto out;
 
+#if IS_ENABLED(CONFIG_NFS_V3_ACL)
 	status = posix_acl_create(dir, &sattr->ia_mode, &default_acl, &acl);
 	if (status)
 		goto out;
+#endif
 
 	data->msg.rpc_proc = &nfs3_procedures[NFS3PROC_MKNOD];
 	data->arg.mknod.fh = NFS_FH(dir);