Re: [PATCH v2 0/4] btrfs: define and apply the AUTO_K(V)FREE_PTR macros

[Miquel Sabaté Solà]

David Sterba @ 2025-10-31 03:22 +01:

> 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@xxxxxxxxxxxxx/
>> [2] https://lore.kernel.org/all/20250925184139.403156-1-mssola@xxxxxxxxxx/
>> [3] https://lore.kernel.org/all/20250930130452.297576-1-mssola@xxxxxxxxxx/
>> [4] https://lore.kernel.org/all/20251008121859.440161-1-mssola@xxxxxxxxxx/
>>
>> 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.

Thanks for adding them!

Will keep an eye then if there are places where it's safe to use them.

Greetings,
Miquel

Attachment: signature.asc
Description: PGP signature