Re: [PATCH v2 1/3] btrfs: DEFINE_FREE for btrfs_free_path

[Boris Burkov <boris@bur.io>]

On Thu, Aug 29, 2024 at 07:36:56PM +0200, David Sterba wrote:
> On Wed, Aug 28, 2024 at 11:54:42AM -0700, Boris Burkov wrote:
> > > > This pattern came from the cleanup.h documentation:
> > > > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L11
> > > 
> > > [...] @free should typically include a NULL test before calling a function
> > > 
> > > Typically yes, but we don't need to do it twice.
> > 
> > I believe we do if we want to get the desired compiler behavior in the
> > release case. Whether or not the resource-freeing function we call
> > checks NULL is not relevant to the point of checking it here in the
> > macro.
> 
> I'm trying to understand why we're discussing that, maybe I'm missing
> some aspect that makes it important to stick to the recommended use.
> I've been reading the macros and looking for potential use, from that
> POV no "if(NULL)" check is needed when it's done in the freeing
> function.

For the record, I I agree that there is no risk for ever doing something
bad to a btrfs_path, no matter what we do with this extra check in
DEFINE_FREE

> 
> There will be few cases that we will review when using the macros and
> then we can forget about the details and it will work.
> 
> > > > As far as I can tell, it will only be relevant if we end up using the
> > > > 'return_ptr' functionality in the cleanup library, which seems
> > > > relatively unlikely for btrfs_path.
> > > 
> > > So return_ptr undoes __free, in that case we shouldn't use it at all,
> > > the macros obscure what the code does, this is IMHO taking it too far.
> > 
> > This may well be taking it too far, but it is a common and valid
> > pattern of RAII: auto freeing the half-initialized parts of structure
> > automatically on the error exit paths in the initialization, while
> > releasing the local cleanup responsibility on success.
> > 
> > Look at their alloc_obj example again:
> > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L31
> > and the explanation that acknowledges kfree handles NULL:
> > https://github.com/torvalds/linux/blob/master/include/linux/cleanup.h#L43
> > 
> > Suppose we were initializing some object that owned a path (and cleaned
> > it up itself on death), then initializing that object we would create a
> > __free owned path (to cleanup on every error path), but then once we were
> > sure we were done, we would release the auto free and let the owning object
> > take over before returning it to the caller. Freeing the path in this case
> > would be a bug, as the owning object would have it freed under it.
> > 
> > That's almost certainly nonsense for btrfs_path and will never happen,
> > but it isn't in general,
> 
> You got me worried in the previous paragraph, I agree it will never
> happen so I'm less inclined to prepare for such cases.
> 

That is fair enough, and I have no problem with that judgement.

> > so if we do add a __free, it makes sense to me
> > to do it by the book. If we really want to avoid this double check, then
> > we should add a comment saying that btrfs_path will never be released,
> > so it doesn't make sense to support that pattern.
> 
> Sorry I don't understand this, can you please provide pseudo-code
> examples? Why wouldn't be btrfs_path released?

I think this is just me not having a good terminology for "we used
return_ptr or no_free_ptr". Perhaps "gave up ownership" or "gave up
responsibility" or "canceled free" as "released" is too similar to the
actual __free action :)

I don't have any pseudocode materially different from the example in
cleanup.h with "btrfs_path" substituted in.

All I'm trying to accomplish in this whole discussion is emphasize that
the extra IS_ERR_OR_NULL check is not about ensuring that btrfs_path is
valid, but is about following the best practice for supporting the code
reduction in the "gave up ownership" happy path. In fact, just "if (_T)"
would be sufficient in Leo's DEFINE_FREE, for that reason.

My taste preference here is to explicitly acknowledge that we plan to
never give up ownership of an auto-free btrfs_path, and thus document
that we are intentionally not including the extra NULL check. Since the
authors of the library bothered to explicitly call it out as a pattern
users should follow.

Thanks for entertaining this discussion, I enjoyed learning more about
cleanup.h.

Boris