[PATCH v4 0/5] code cleanups for btrfs_get_acl()

[PATCH v4 0/5] code cleanups for btrfs_get_acl()

[Chengguang Xu]

This patch set does code cleanups for btrfs_get_acl().

Chengguang Xu (5):
  btrfs: return error instead of crash when detecting unexpected type in
    btrfs_get_acl()
  btrfs: replace empty string with NULL when getting attribute length in
    btrfs_get_acl()
  btrfs: remove unnecessary -ERANGE check in btrfs_get_acl()
  btrfs: avoid error code overriding in btrfs_get_acl()
  btrfs: remove unnecessary bracket in btrfs_get_acl()

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

-- 
2.17.1

[PATCH v4 1/5] btrfs: return error instead of crash when detecting unexpected type in btrfs_get_acl()

[Chengguang Xu]

The caller of btrfs_get_acl() checks error condition so there is no
impact from this change. In practice there is no chance to get into
default case of switch statement because VFS has already checked
the type.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v4:
- Split patch to series.

v3:
- Fix some toher bad practices.
- Add more information to commit log.

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

 fs/btrfs/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 15e1dfef56a5..60f83a3bd77c 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -30,7 +30,7 @@ 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);
-- 
2.17.1

[PATCH v4 2/5] btrfs: replace empty string with NULL when getting attribute length in btrfs_get_acl()

[Chengguang Xu]

In btrfs_get_acl() the first call of btr_getxattr() is for getting
the length of attribute, the value buffer is never used in this
case. So it's better to replace empty string with NULL.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v4:
- Split patch to series.

v3:
- Fix some toher bad practices.
- Add more information to commit log.

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

 fs/btrfs/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 60f83a3bd77c..83fdd80c51c6 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -33,7 +33,7 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 		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)
-- 
2.17.1

[PATCH v4 3/5] btrfs: remove unnecessary -ERANGE check in 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.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v4:
- Split patch to series.

v3:
- Fix some toher bad practices.
- Add more information to commit log.

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

 fs/btrfs/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 83fdd80c51c6..a1d7211c8884 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -42,7 +42,7 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 	}
 	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);
-- 
2.17.1

[PATCH v4 4/5] btrfs: avoid error code overriding in btrfs_get_acl()

[Chengguang Xu]

It's no good to override error code when failing from
btrfs_getxattr() in btrfs_get_acl() because it will hide
real root cause.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v4:
- Split patch to series.

v3:
- Fix some toher bad practices.
- Add more information to commit log.

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

 fs/btrfs/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index a1d7211c8884..7d673ec9e54a 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -45,7 +45,7 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 	} else if (size == -ENODATA || size == 0) {
 		acl = NULL;
 	} else {
-		acl = ERR_PTR(-EIO);
+		acl = ERR_PTR(size);
 	}
 	kfree(value);
 
-- 
2.17.1

[PATCH v4 5/5] btrfs: remove unnecessary bracket in btrfs_get_acl()

[Chengguang Xu]

It's only coding style fix not functinal change.
When if/else has only one statement then the bracket
is not needed.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v4:
- Split patch to series.

v3:
- Fix some toher bad practices.
- Add more information to commit log.

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

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

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 7d673ec9e54a..3b66c957ea6f 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -40,13 +40,12 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 			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 == -ENODATA || size == 0) {
+	else if (size == -ENODATA || size == 0)
 		acl = NULL;
-	} else {
+	else
 		acl = ERR_PTR(size);
-	}
 	kfree(value);
 
 	return acl;
-- 
2.17.1

Re: [PATCH v4 0/5] code cleanups for btrfs_get_acl()

[David Sterba]

On Wed, Jun 27, 2018 at 12:16:33PM +0800, Chengguang Xu wrote:
> This patch set does code cleanups for btrfs_get_acl().
> 
> Chengguang Xu (5):
>   btrfs: return error instead of crash when detecting unexpected type in
>     btrfs_get_acl()
>   btrfs: replace empty string with NULL when getting attribute length in
>     btrfs_get_acl()
>   btrfs: remove unnecessary -ERANGE check in btrfs_get_acl()
>   btrfs: avoid error code overriding in btrfs_get_acl()
>   btrfs: remove unnecessary bracket in btrfs_get_acl()

Perfect, that's exactly what I expected. Thanks.