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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux