Re: [PATCH v2 08/11] reftable/writer: unify releasing memory

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 4, 2024, 07:32 UTC
Message-ID: <Zg5Xorz75NMRqONI@tanuki>
In-Reply-To: <CAOw_e7aBPF1vPvF7iYXCM5VBQu-Nw00dO2pRC_6DU3PtdDUsbg@mail.gmail.com>

On Thu, Apr 04, 2024 at 09:08:46AM +0200, Han-Wen Nienhuys wrote:

Show 17 quoted lines

> On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt <ps@pks.im> wrote:
> > There are two code paths which release memory of the reftable writer:
> >
> >   - `reftable_writer_close()` releases internal state after it has
> >     written data.
> >
> >   - `reftable_writer_free()` releases the block that was written to and
> >     the writer itself.
> 
> The bifurcation is there so you can read the stats after closing the
> writer. The new method makes it harder to misuse, but now you have two
> ways to end a writer. Suggestion: drop reftable_writer_{free,close}
> from reftable-writer.h (rename to remove the reftable_ prefix because
> they are no longer considered public) and find another way to read out
> the stats. Either pass an optional reftable_writer_stats into the
> construction of the writer, return the stats from the close function,
> or drop stats altogether.  IIRC They are only used in the unit tests.

But even with these refactorings the stats remain intact after calling `reftable_writer_close()` or `reftable_writer_release()`, right? So it basically continues to work as expected.

It might not be the cleanest way to handle this, but I think this patch is an improvement over the previous state because we plug a memory leak and deduplicate the cleanup logic. So I would suggest to defer your proposed refactorings to a later point, if you're okay with that.

Thanks!

Patrick