On Sat, Feb 23, 2019 at 01:39:52AM +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.
>
> Signed-off-by: Anand Jain <anand.j...@oracle.com>
> ---
> v4: born
> fs/btrfs/acl.c | 9 ++++++++-
> fs/btrfs/props.c | 16 ++++++++++------
> fs/btrfs/xattr.c | 30 ++++++++----------------------
> fs/btrfs/xattr.h | 5 ++---
> 4 files changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index e395615dd304..75abe7f0d05d 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;
> }
>
> - ret = btrfs_setxattr(trans, inode, name, value, size, 0);
> + if (btrfs_root_readonly(root)) {
> + ret = -EROFS;
> + goto out;
> + }
> +
> + ret = do_setxattr(trans, inode, name, value, size, 0);
> +
> out:
> kfree(value);
>
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index f878ba3160f0..be6d16ccc738 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -60,6 +60,7 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans,
> struct inode *inode,
> const char *name, const char *value, size_t value_len,
> int flags)
> {
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> const struct prop_handler *handler;
> int ret;
>
> @@ -70,9 +71,12 @@ static int btrfs_set_prop(struct btrfs_trans_handle
> *trans, struct inode *inode,
> if (!handler)
> return -EINVAL;
>
> + if (btrfs_root_readonly(root))
> + return -EROFS;
> +
> if (value_len == 0) {
> - ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> - flags);
> + ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> + flags);
> 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 = do_setxattr(trans, inode, handler->xattr_name, value, value_len,
> + flags);
> if (ret)
> return ret;
> ret = handler->apply(inode, value, value_len);
> if (ret) {
> - btrfs_setxattr(trans, inode, handler->xattr_name,
> - NULL, 0, flags);
> + ret = do_setxattr(trans, inode, handler->xattr_name, NULL, 0,
> + flags);
> return ret;
> }
>
> diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
> index b3281d4d95b9..4e1ccfbfa44a 100644
> --- a/fs/btrfs/xattr.c
> +++ b/fs/btrfs/xattr.c
> @@ -76,9 +76,8 @@ int btrfs_getxattr(struct inode *inode, const char *name,
> return ret;
> }
>
> -static int do_setxattr(struct btrfs_trans_handle *trans,
> - struct inode *inode, const char *name,
> - const void *value, size_t size, int flags)
> +int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
> + const char *name, const void *value, size_t size, int flags)
> {
> struct btrfs_dir_item *di = NULL;
> struct btrfs_root *root = BTRFS_I(inode)->root;
> @@ -217,22 +216,6 @@ static int do_setxattr(struct btrfs_trans_handle *trans,
> return ret;
> }
>
> -/*
> - * @value: "" makes the attribute to empty, NULL removes it
> - */
> -int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
> - const char *name, const void *value, size_t size, int flags)
> -{
> - struct btrfs_root *root = BTRFS_I(inode)->root;
> -
> - ASSERT(!trans);
> -
> - if (btrfs_root_readonly(root))
> - return -EROFS;
> -
> - return do_setxattr(trans, inode, name, value, size, flags);
> -}
> -
> ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
> {
> struct btrfs_key key;
> @@ -357,11 +340,14 @@ static int btrfs_xattr_handler_set(const struct
> xattr_handler *handler,
>
> name = xattr_full_name(handler, name);
>
> + if (btrfs_root_readonly(root))
> + return -EROFS;
> +
> trans = btrfs_start_transaction(root, 2);
> if (IS_ERR(trans))
> return PTR_ERR(trans);
>
> - ret = btrfs_setxattr(trans, inode, name, buffer, size, flags);
> + ret = do_setxattr(trans, inode, name, buffer, size, flags);
>
> if (!ret) {
> inode_inc_iversion(inode);
> @@ -444,8 +430,8 @@ static int btrfs_initxattrs(struct inode *inode,
> }
> strcpy(name, XATTR_SECURITY_PREFIX);
> strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);
> - err = btrfs_setxattr(trans, inode, name, xattr->value,
> - xattr->value_len, 0);
> + err = do_setxattr(trans, inode, name, xattr->value,
> + xattr->value_len, 0);
> kfree(name);
> if (err < 0)
> break;
> diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h
> index 471fcac6ff55..276f81a622d5 100644
> --- a/fs/btrfs/xattr.h
> +++ b/fs/btrfs/xattr.h
> @@ -12,9 +12,8 @@
>
> int btrfs_getxattr(struct inode *inode, const char *name,
> void *buffer, size_t size);
> -int btrfs_setxattr(struct btrfs_trans_handle *trans,
> - struct inode *inode, const char *name,
> - const void *value, size_t size, int flags);
> +int do_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
> + const char *name, const void *value, size_t size, int flags);
No, this is backwards. Why do you promote a static helper to interface
function but without the btrfs_ prefix?
The point of do_something functions is to actually do the thing assuming
that external conditions have been checked, like the readonly status.
All such checks should happen at the entry, ie. beginning of the various
callbacks.
