Message-ID: <20251031022241.GE13846@twin.jikos.cz>
Date: Fri, 31 Oct 2025 03:22:41 +0100
From: David Sterba <dsterba@...e.cz>
To: Miquel Sabaté Solà <mssola@...ola.com>
Cc: linux-btrfs@...r.kernel.org, clm@...com, dsterba@...e.com,
	johannes.thumshirn@....com, fdmanana@...e.com, boris@....io,
	wqu@...e.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] btrfs: define and apply the AUTO_K(V)FREE_PTR
 macros

On Fri, Oct 24, 2025 at 12:21:39PM +0200, Miquel Sabaté Solà wrote:
> Changes since v1:
>   - Remove the _PTR suffix
>   - Rename the ipath cleanup function to inode_fs_paths, so it's more
>     explicit on the type.
>   - Improve git message in patch 1.
> 
> This patchset introduces and applies throughout the btrfs tree two new
> macros: AUTO_KFREE and AUTO_KVFREE. Each macro defines a pointer,
> initializes it to NULL, and sets the kfree/kvfree cleanup attribute. It was
> suggested by David Sterba in the review of a patch that I submitted here
> [1].
> 
> I have not applied these macros blindly through the tree, but only when
> using a cleanup attribute actually made things easier for
> maintainers/developers, and didn't obfuscate things like lifetimes of
> objects on a given function. So, I've mostly avoided applying this when:
> 
> - The object was being re-allocated in the middle of the function
>   (e.g. object re-allocation in a loop).
> - The ownership of the object was transferred between functions.
> - The value of a given object might depend on functions returning ERR_PTR()
>   et al.
> - The cleanup section of a function was a bunch of labels with different
>   exit paths with non-trivial cleanup code (or code that depended on things
>   to go on a specific order).
> 
> To come up with this patchset I have glanced through the tree in order to
> find where and how kfree()/kvfree() were being used, and while doing so I
> have submitted [2], [3] and [4] separately as they were fixing memory
> related issues. All in all, this patchset can be divided in three parts:
> 
> 1. Patch 1: transforms free_ipath() to be defined via DEFINE_FREE(), which
>    will be useful in order to further simplify some code in patch 3.
> 2. Patch 2 and 3: define and use the two macros.
> 3. Patch 4: removing some unneeded kfree() calls from qgroup.c as they were
>    not needed. Since these occurrences weren't memory bugs, and it was a
>    somewhat simple patch, I've refrained from sending this separately as I
>    did in [2], [3] and [4]; but I'll gladly do it if you think it's better
>    for the review.
> 
> Note that after these changes some 'return' statements could be made more
> explicit, and I've also written an explicit 'return 0' whenever it would
> make more explicit the "happy" path for a given branch, or whenever a 'ret'
> variable could be avoided that way.
> 
> Last, checkpatch.pl script doesn't seem to like patches 2 and 3; but so far
> it looks like false positives to me. But of course I might just be wrong :)
> 
> [1] https://lore.kernel.org/all/20250922103442.GM5333@twin.jikos.cz/
> [2] https://lore.kernel.org/all/20250925184139.403156-1-mssola@mssola.com/
> [3] https://lore.kernel.org/all/20250930130452.297576-1-mssola@mssola.com/
> [4] https://lore.kernel.org/all/20251008121859.440161-1-mssola@mssola.com/
> 
> Miquel Sabaté Solà (4):
>   btrfs: declare free_ipath() via DEFINE_FREE()
>   btrfs: define the AUTO_K(V)FREE helper macros
>   btrfs: apply the AUTO_K(V)FREE macros throughout the tree
>   btrfs: add ASSERTs on prealloc in qgroup functions

Thanks, patches now added to for-next with some minor adjustments. Feel
free to send more conversions, there are still some kvfree candidate
calls. I think we would not mind using it even for the short functions
(re what's mentioned in the 3rd patch), so it's established as a common
coding pattern. This change has net negative effect on lines and also
simplifies the control flow.

Powered by blists - more mailing lists