Re: [PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()

David Sterba
2024-02-19T14:41:59Z

On Mon, Feb 19, 2024 at 12:27:44PM +0000, Filipe Manana wrote:
> On Mon, Feb 19, 2024 at 11:14 AM David Sterba <dsterba@suse.com> wrote:
> >
> > The helper is simple enough for inlining, we can further simplify it by
> > removing the check for map pointer validity. After this patch all
> > callers always pass a valid pointer.
>
> So by making btrfs_free_chunk_map() to not ignore a NULL pointer, we
> are adding rather
> surprising behaviour and inconsistency.
>
> Most free functions ignore a NULL pointer, take the example of the
> kfree() family and even free() family in the standard C library,
> as well as most of the free functions we have in btrfs as well, which
> are modeled on that common pattern.
>
> Ignoring NULL makes error handling simpler, by not having the need to
> take special care to call the free function with a non-NULL pointer.
>
> Besides that, this change doesn't seem to improve anything.

The goal is to reduce a static inline function size as its code is
duplicated many times (in this case 36x) so anything that does not need
to be there is removed. The improvement is smaller code size, one less
condition to check.

OTOH that it's a freeing function and would not accept a NULL pointer is
indeed inconsistent and potentially problematic so I'll drop the patch.