On Fri, Mar 01, 2019 at 12:34:55PM +0800, Anand Jain wrote:
> Now btrfs_setxattr() is a very small function with just check for
> readonly FS and redirect the call to do_setxattr(). So instead
> move that checks to the parent functions and call do_setxattr()
> directly. Delete original btrfs_setxattr(), and rename do_setxattr()
> to btrfs_setxattr(). Also add few c-style. Kindly note the arguments
> of original do_setxattr() and original btrfs_setxattr() are same, so the
> diff obliterates the changes as described above.
>
> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> ---
> v5: rename do_setxattr() to btrfs_setxattr(). change log update. fix c-style.
> v4: born
> fs/btrfs/acl.c | 7 +++++++
> fs/btrfs/props.c | 16 ++++++++++------
> fs/btrfs/xattr.c | 29 +++++++++--------------------
> fs/btrfs/xattr.h | 5 ++---
> 4 files changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index 2b9c3394fc34..a8a1060c8cbe 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -60,6 +60,7 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
> int ret, size = 0;
> const char *name;
> char *value = NULL;
> + struct btrfs_root *root = BTRFS_I(inode)->root;
>
> switch (type) {
> case ACL_TYPE_ACCESS:
> @@ -95,7 +96,13 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode,
> goto out;
> }
>
> + if (btrfs_root_readonly(root)) {
> + ret = -EROFS;
> + goto out;
All the readonly checks needs to go first as it's the global condition,
the following checks make sure that the arguments are valid and depend
on the previous.
> - ret = btrfs_setxattr(trans, inode, handler->xattr_name,
> - NULL, 0, flags);
> + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> + flags);
drive-by change
> if (ret)
> return ret;
>
> @@ -85,14 +89,14 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
> ret = handler->validate(value, value_len);
> if (ret)
> return ret;
> - ret = btrfs_setxattr(trans, inode, handler->xattr_name,
> - value, value_len, flags);
> + ret = btrfs_setxattr(trans, inode, handler->xattr_name, value,
> + value_len, flags);
same
> if (ret)
> return ret;
> ret = handler->apply(inode, value, value_len);
> if (ret) {
> - btrfs_setxattr(trans, inode, handler->xattr_name,
> - NULL, 0, flags);
> + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> + flags);
And that one silently changes the return value semantics but looks like
the other two, "just fixing the indentation". The original code does not
set 'ret' as the whole operation returns what the property handler
returned.
The setxattr call here resets the property. If this is not the
right, then fixed separately.
