Re: [PATCH v0 17/18] btrfs: add qgroup ioctls

[Andi Kleen <andi@firstfloor.org>]

Arne Jansen <sensille@gmx.net> writes:
> +
> +	if (copy_to_user(arg, sa, sizeof(*sa)))
> +		ret = -EFAULT;
> +
> +	if (trans) {
> +		err = btrfs_commit_transaction(trans, root);
> +		if (err && !ret)
> +			ret = err;
> +	}

It would seem safer to put the copy to user outside the transaction.
A cto can in principle cause new writes (e.g. if it causes COW), so 
you may end up with nested transactions. Even if that works somehow
(not sure) it seems to be a thing better avoided.

> +
> +	sa = memdup_user(arg, sizeof(*sa));
> +	if (IS_ERR(sa))
> +		return PTR_ERR(sa);
> +
> +	trans = btrfs_join_transaction(root);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}

This code seems to be duplicated a lot. Can it be consolidated?

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only