[PATCH v3] btrfs: code cleanups for btrfs_get_acl()

[PATCH v3] btrfs: code cleanups for btrfs_get_acl()

[Chengguang Xu]

There is no chance to get into -ERANGE error condition because
we first call btrfs_getxattr to get the length of the attribute,
then we do a subsequent call with the size from the first call.
Between the 2 calls the size shouldn't change. So remove the
unnecessary -ERANGE error check and meanwhile fix some other
bad practices as well.

Detail fixes in this patch.
- return ERR_PTR(-EINVAL) instead of crashing kernel in default
case of switch.
- return original error code when failing from btrfs_getxattr().
- remove unnecessary bracket.
- remove unnecessary -ERANGE check.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
v3:
- Fix some other bad practices.
- Add more information to commit log.

v2:
- Avoid errno overriding instead of print error message in error case.
- Change commit log for better understanding.

 fs/btrfs/acl.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 15e1dfef56a5..3b66c957ea6f 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -30,23 +30,22 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 		name = XATTR_NAME_POSIX_ACL_DEFAULT;
 		break;
 	default:
-		BUG();
+		return ERR_PTR(-EINVAL);
 	}
 
-	size = btrfs_getxattr(inode, name, "", 0);
+	size = btrfs_getxattr(inode, name, NULL, 0);
 	if (size > 0) {
 		value = kzalloc(size, GFP_KERNEL);
 		if (!value)
 			return ERR_PTR(-ENOMEM);
 		size = btrfs_getxattr(inode, name, value, size);
 	}
-	if (size > 0) {
+	if (size > 0)
 		acl = posix_acl_from_xattr(&init_user_ns, value, size);
-	} else if (size == -ERANGE || size == -ENODATA || size == 0) {
+	else if (size == -ENODATA || size == 0)
 		acl = NULL;
-	} else {
-		acl = ERR_PTR(-EIO);
-	}
+	else
+		acl = ERR_PTR(size);
 	kfree(value);
 
 	return acl;
-- 
2.17.1

Re: [PATCH v3] btrfs: code cleanups for btrfs_get_acl()

[Nikolay Borisov]

On 26.06.2018 09:08, Chengguang Xu wrote:
> There is no chance to get into -ERANGE error condition because
> we first call btrfs_getxattr to get the length of the attribute,
> then we do a subsequent call with the size from the first call.
> Between the 2 calls the size shouldn't change. So remove the
> unnecessary -ERANGE error check and meanwhile fix some other
> bad practices as well.
> 
> Detail fixes in this patch.
> - return ERR_PTR(-EINVAL) instead of crashing kernel in default
> case of switch.
> - return original error code when failing from btrfs_getxattr().
> - remove unnecessary bracket.
> - remove unnecessary -ERANGE check.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> v3:
> - Fix some other bad practices.
> - Add more information to commit log.
> 
> v2:
> - Avoid errno overriding instead of print error message in error case.
> - Change commit log for better understanding.
> 
>  fs/btrfs/acl.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index 15e1dfef56a5..3b66c957ea6f 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -30,23 +30,22 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
>  		name = XATTR_NAME_POSIX_ACL_DEFAULT;
>  		break;
>  	default:
> -		BUG();
> +		return ERR_PTR(-EINVAL);
>  	}
>  
> -	size = btrfs_getxattr(inode, name, "", 0);
> +	size = btrfs_getxattr(inode, name, NULL, 0);
>  	if (size > 0) {
>  		value = kzalloc(size, GFP_KERNEL);
>  		if (!value)
>  			return ERR_PTR(-ENOMEM);
>  		size = btrfs_getxattr(inode, name, value, size);
>  	}
> -	if (size > 0) {
> +	if (size > 0)
>  		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -	} else if (size == -ERANGE || size == -ENODATA || size == 0) {
> +	else if (size == -ENODATA || size == 0)
>  		acl = NULL;
> -	} else {
> -		acl = ERR_PTR(-EIO);
> -	}
> +	else
> +		acl = ERR_PTR(size);
>  	kfree(value);
>  
>  	return acl;
> 

Re: [PATCH v3] btrfs: code cleanups for btrfs_get_acl()

[David Sterba]

On Tue, Jun 26, 2018 at 02:08:27PM +0800, Chengguang Xu wrote:
> There is no chance to get into -ERANGE error condition because
> we first call btrfs_getxattr to get the length of the attribute,
> then we do a subsequent call with the size from the first call.
> Between the 2 calls the size shouldn't change. So remove the
> unnecessary -ERANGE error check and meanwhile fix some other
> bad practices as well.
> 
> Detail fixes in this patch.
> - return ERR_PTR(-EINVAL) instead of crashing kernel in default
> case of switch.
> - return original error code when failing from btrfs_getxattr().
> - remove unnecessary bracket.
> - remove unnecessary -ERANGE check.

Hm, that should be really 3 patches, but Nikolay is to blame as he
should have mentioned that when asking for the other things to fix.

> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
> v3:
> - Fix some other bad practices.
> - Add more information to commit log.
> 
> v2:
> - Avoid errno overriding instead of print error message in error case.
> - Change commit log for better understanding.
> 
>  fs/btrfs/acl.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index 15e1dfef56a5..3b66c957ea6f 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -30,23 +30,22 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
>  		name = XATTR_NAME_POSIX_ACL_DEFAULT;
>  		break;
>  	default:
> -		BUG();
> +		return ERR_PTR(-EINVAL);

First patch

>  	}
>  
> -	size = btrfs_getxattr(inode, name, "", 0);
> +	size = btrfs_getxattr(inode, name, NULL, 0);

Second patch

>  	if (size > 0) {
>  		value = kzalloc(size, GFP_KERNEL);
>  		if (!value)
>  			return ERR_PTR(-ENOMEM);
>  		size = btrfs_getxattr(inode, name, value, size);
>  	}
> -	if (size > 0) {
> +	if (size > 0)
>  		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -	} else if (size == -ERANGE || size == -ENODATA || size == 0) {
> +	else if (size == -ENODATA || size == 0)
>  		acl = NULL;
> -	} else {
> -		acl = ERR_PTR(-EIO);
> -	}
> +	else
> +		acl = ERR_PTR(size);

And third patch.

>From current changelog only the 3rd is covered, otherwise 1st and 2nd
are under the "and other bad things", but this deserves a more specific
explanation.

So please split and resend. Thanks.