Re: v2.46.0-rc0 test failures on cygwin

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

 



On Tue, Jul 23, 2024 at 12:30:27PM +0200, Patrick Steinhardt wrote:

> > So my biggest question is just whether there is any downside to doing
> > the release/init pair rather than trying to reuse the existing struct.
> 
> There shouldn't be any downside, but it is wasted effort. The main ref
> store should always be accessed via `get_main_ref_store()`, and that
> function knows to lazily initialize the refdb as required. So instead, I
> think the preferable fix is to release the new ref store after we have
> populated it and set the main ref store of the repository to `NULL`
> instead of re-initializing it.

Ah, that is even better. I'm not too worried about wasted effort (this
is a one-time thing during the ref backend migration), but I think we
are better off leaving as much to the "regular" ref code as possible.

> So the following should be sufficient:
> [...]

Yeah, that looks nice.

> > I do think it probably causes a small memory leak. The "init" function
> > allocates the actual ref_store struct, but the "release" function
> > doesn't seem to free it. So we are probably leaking the store that
> > points to the temp directory. But that is also true of "old_refs", or of
> > "new_refs" if we hit an error. So I think the solution is probably for
> > init/release to be symmetric, and for the latter to clean up everything.
> > But again, I'd prefer to get input from Patrick there.
> 
> Yeah, we'd have to free the new ref store struct, as well. I wouldn't
> make `release()` free the structure as that would be uncustomarily
> named.

We usually don't free in the release() function because it was not
allocated on the init() side in the first place. The real sin here is
that the init/release pair is not symmetric, regardless of what they are
named.

In the patch above you worked around it by just doing a manual free() of
the struct. That's crossing the abstraction barrier a little bit, but I
think is OK in this instance. We don't generally expect a ref stores to
be created and released a lot. If this were a general purpose data
structure like a strbuf that gets used everywhere, I'd be more
concerned.

-Peff




[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