| 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 |
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.
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.
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 --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);
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(-)