* [PATCH v3] btrfs: code cleanups for btrfs_get_acl()
@ 2018-06-26 6:08 Chengguang Xu
2018-06-26 15:51 ` Nikolay Borisov
2018-06-26 16:19 ` David Sterba
0 siblings, 2 replies; 3+ messages in thread
From: Chengguang Xu @ 2018-06-26 6:08 UTC (permalink / raw)
To: clm, jbacik, dsterba; +Cc: linux-btrfs, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] btrfs: code cleanups for btrfs_get_acl()
2018-06-26 6:08 [PATCH v3] btrfs: code cleanups for btrfs_get_acl() Chengguang Xu
@ 2018-06-26 15:51 ` Nikolay Borisov
2018-06-26 16:19 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: Nikolay Borisov @ 2018-06-26 15:51 UTC (permalink / raw)
To: Chengguang Xu, clm, jbacik, dsterba; +Cc: linux-btrfs
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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] btrfs: code cleanups for btrfs_get_acl()
2018-06-26 6:08 [PATCH v3] btrfs: code cleanups for btrfs_get_acl() Chengguang Xu
2018-06-26 15:51 ` Nikolay Borisov
@ 2018-06-26 16:19 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: David Sterba @ 2018-06-26 16:19 UTC (permalink / raw)
To: Chengguang Xu; +Cc: clm, jbacik, linux-btrfs, nborisov
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-26 16:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 6:08 [PATCH v3] btrfs: code cleanups for btrfs_get_acl() Chengguang Xu
2018-06-26 15:51 ` Nikolay Borisov
2018-06-26 16:19 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.