On Tue, Oct 21, 2025 at 04:27:45PM +0200, Miquel Sabaté Solà wrote:
> This patchset introduces and applies throughout the btrfs tree two new
> macros: AUTO_KFREE_PTR and AUTO_KVFREE_PTR. 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 added the _PTR suffix because I think that it reads more easily
> and it makes things more explicit, but I don't have any strong feelings
> about the naming either.
I'd rather drop the _PTR part as it's not bringing new information
(kfree takes a pointer) and it makes the lines longer.
> 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).
This looks reasonable, some of them may be relaxed eventually.
> 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 :)
We take checkpatch as a recommendation but still a good base line, the
local btrfs coding style may diverge and is fixed manually at commit
time.