Re: [PATCH v4 10/10] btrfs: kill btrfs_setxattr

[Anand Jain <anand.jain@oracle.com>]

On 2/28/19 12:21 AM, David Sterba wrote:
> 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.jain@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.

  I though so as well. Will rename.

Thanks, Anand