Re: [PATCH v5 9/9] btrfs: kill btrfs_setxattr

[Anand Jain <anand.jain@xxxxxxxxxx>]

On 3/8/19 10:56 PM, David Sterba wrote:
> 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.

 That's copy and paste error. :-(. It should return ret of the
 handler->apply(). Because if the undo part which is
 btrfs_setxattr() is successful it means we fail silently.
 But at this place the handler->apply() can not fail. So the
 fail part is only theoretical.

 Also, theoretically in the original code the undo part is bit wrong,
 instead of resetting to the NULL it should reset back to the
 old value.

 I was trying to read the patch which is integrated if you
 have made any changes, so that I can send the fix. But I
 can't seems to find them.

Thanks, Anand