On Tue, Apr 19, 2022 at 07:47:09PM +0800, Yang Xu wrote:
> Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
> don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.
>
> The previous patch has added missing umask strip for tmpfile, so all creation
> paths handle umask in the vfs directly if the filesystem doesn't support or
> enable POSIX ACLs.
>
> So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
> well.
>
> Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
> xfs_generic_create.
>
> Signed-off-by: Yang Xu <[email protected]>
> ---
> fs/xfs/xfs_iops.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b34e8e4344a8..6b8df9ab215a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -150,6 +150,7 @@ xfs_create_need_xattr(
> return true;
> if (default_acl)
> return true;
> +
> #if IS_ENABLED(CONFIG_SECURITY)
> if (dir->i_sb->s_security)
> return true;
> @@ -169,7 +170,7 @@ xfs_generic_create(
> {
> struct inode *inode;
> struct xfs_inode *ip = NULL;
> - struct posix_acl *default_acl, *acl;
> + struct posix_acl *default_acl = NULL, *acl = NULL;
> struct xfs_name name;
> int error;
>
> @@ -184,9 +185,11 @@ xfs_generic_create(
> rdev = 0;
> }
>
> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
> error = posix_acl_create(dir, &mode, &default_acl, &acl);
> if (error)
> return error;
> +#endif
Does this actually fix or improve anything?
If CONFIG_XFS_POSIX_ACL isn't selected then SB_POSIXACL won't be set in
inode->i_sb->s_flags and consequently posix_acl_create() is a nop. So
ifdefing this doesn't really do anything so I'd argue to not bother with
this change.
> /* Verify mode is valid also for tmpfile case */
> error = xfs_dentry_mode_to_name(&name, dentry, mode);
> @@ -209,7 +212,7 @@ xfs_generic_create(
> if (unlikely(error))
> goto out_cleanup_inode;
>
> -#ifdef CONFIG_XFS_POSIX_ACL
> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
> if (default_acl) {
> error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> if (error)
Side-note, I think
#ifdef CONFIG_XFS_POSIX_ACL
extern struct posix_acl *xfs_get_acl(struct inode *inode, int type, bool rcu);
extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
struct posix_acl *acl, int type);
extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
#else
extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
struct posix_acl *acl, int type)
{
return 0;
}
extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
{
return 0;
}
#endif
and then removing the inline-ifdef might be an improvement.
on 2022/4/19 21:53, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:09PM +0800, Yang Xu wrote:
>> Since xfs_generic_create only calls xfs_set_acl when enable this kconfig, we
>> don't need to call posix_acl_create for the !CONFIG_XFS_POSIX_ACL case.
>>
>> The previous patch has added missing umask strip for tmpfile, so all creation
>> paths handle umask in the vfs directly if the filesystem doesn't support or
>> enable POSIX ACLs.
>>
>> So just put this function under CONFIG_XFS_POSIX_ACL and umask strip still works
>> well.
>>
>> Also use unified rule for CONFIG_XFS_POSIX_ACL in this file, so use IS_ENABLED in
>> xfs_generic_create.
>>
>> Signed-off-by: Yang Xu<[email protected]>
>> ---
>> fs/xfs/xfs_iops.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index b34e8e4344a8..6b8df9ab215a 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -150,6 +150,7 @@ xfs_create_need_xattr(
>> return true;
>> if (default_acl)
>> return true;
>> +
>> #if IS_ENABLED(CONFIG_SECURITY)
>> if (dir->i_sb->s_security)
>> return true;
>> @@ -169,7 +170,7 @@ xfs_generic_create(
>> {
>> struct inode *inode;
>> struct xfs_inode *ip = NULL;
>> - struct posix_acl *default_acl, *acl;
>> + struct posix_acl *default_acl = NULL, *acl = NULL;
>> struct xfs_name name;
>> int error;
>>
>> @@ -184,9 +185,11 @@ xfs_generic_create(
>> rdev = 0;
>> }
>>
>> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>> error = posix_acl_create(dir,&mode,&default_acl,&acl);
>> if (error)
>> return error;
>> +#endif
>
> Does this actually fix or improve anything?
> If CONFIG_XFS_POSIX_ACL isn't selected then SB_POSIXACL won't be set in
> inode->i_sb->s_flags and consequently posix_acl_create() is a nop. So
> ifdefing this doesn't really do anything so I'd argue to not bother with
> this change.
It only avoid useless mode &= ~current_mask here.
>
>> /* Verify mode is valid also for tmpfile case */
>> error = xfs_dentry_mode_to_name(&name, dentry, mode);
>> @@ -209,7 +212,7 @@ xfs_generic_create(
>> if (unlikely(error))
>> goto out_cleanup_inode;
>>
>> -#ifdef CONFIG_XFS_POSIX_ACL
>> +#if IS_ENABLED(CONFIG_XFS_POSIX_ACL)
>> if (default_acl) {
>> error = __xfs_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
>> if (error)
>
> Side-note, I think
>
> #ifdef CONFIG_XFS_POSIX_ACL
> extern struct posix_acl *xfs_get_acl(struct inode *inode, int type, bool rcu);
> extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> struct posix_acl *acl, int type);
> extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> #else
> extern int xfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode,
> struct posix_acl *acl, int type)
> {
> return 0;
> }
>
> extern int __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> {
> return 0;
> }
> #endif
>
> and then removing the inline-ifdef might be an improvement.
Maybe, but it should not be in this patchset as you said.
Best Regards
Yang Xu