On Thu, Apr 4, 2024 at 9:32 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > On Thu, Apr 04, 2024 at 09:08:46AM +0200, Han-Wen Nienhuys wrote: > > On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt <ps@xxxxxx> 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. Right - I misinterpreted your change. > 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. yes. Please add reftable_writer_release to reftable-writer.h for consistency, though. Or remove the reftable_ prefix. -- Han-Wen Nienhuys - hanwenn@xxxxxxxxx - http://www.xs4all.nl/~hanwen