Re: [PATCH 0/2] btrfs: add __free attribute and improve xattr cleanup

[David Sterba <dsterba@suse.cz>]

On Thu, Aug 15, 2024 at 10:38:24AM -0700, Leo Martins wrote:
> On Tue, 13 Aug 2024 14:29, David Sterba <dsterba@suse.cz> wrote:
> >On Fri, Aug 09, 2024 at 04:11:47PM -0700, Leo Martins wrote:

> This makes sense, I will drop the xattr patch. Do you think there would
> be any benefit in using the __free pattern in situations where it
> is clear that btrfs_free_path is the last thing called before returning?
> For example:
> 
> 
> int btrfs_del_orphan_item(struct btrfs_trans_handle *trans,
> 			  struct btrfs_root *root, u64 offset)
> {
> 	struct btrfs_path *path;
> 	struct btrfs_key key;
> 	int ret = 0;
> 
> 	key.objectid = BTRFS_ORPHAN_OBJECTID;
> 	key.type = BTRFS_ORPHAN_ITEM_KEY;
> 	key.offset = offset;
> 
> 	path = btrfs_alloc_path();
> 	if (!path)
> 		return -ENOMEM;
> 
> 	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
> 	if (ret < 0)
> 		goto out;
> 	if (ret) { /* JDM: Really? */
> 		ret = -ENOENT;
> 		goto out;
> 	}
> 
> 	ret = btrfs_del_item(trans, root, path);
> 
> out:
> 	btrfs_free_path(path);
> 	return ret;
> }
> 
> 
> In this code the behavior would be the same except it would eliminate
> the need for goto out as the path is freed automatically on exit.

Yes, this is where I coudl be used, basically it's a pattern where the
lock/allocation is done early at the beginning of a function (after some
initial checks) and then right before a return and all exit paths lead
there.

For that I'd suggest to make it clear that the function uses the
automatic unlock/deletion so the declaration of the variable would be
done like

  BTRFS_PATH_AUTOCLEAN(name);

that declares it with the proper __free callback and initializes it to
NULL.

There's another thing that's a common pattern in btrfs and other kernel,
code, the single exit block. The __free callback allows to do a return
anywhere which is the opposite of that. As this is new we should look up
good examples that will be the patterns to follow or exceptions to
avoidd so we can declare it current best practice and recommended coding
style.