Re: [PATCH] daemon: use strbuf for hostname info

From: Jeff King <peff@peff.net>
Date: Mar 7, 2015, 01:07 UTC
Message-ID: <20150307010702.GA8202@peff.net>
In-Reply-To: <54FA4C34.6080702@web.de>

On Sat, Mar 07, 2015 at 01:54:12AM +0100, René Scharfe wrote:

Show 11 quoted lines

> >These probably want to all be strbuf_release(). Again, I doubt it
> >matters much because this is a forked daemon serving only a single
> >request (so they'll get freed by the OS soon anyway), but I think
> >freeing the memory here follows the original intent.
> 
> Using a static strbuf means (in my mind) "don't worry about freeing,
> a memory leak won't happen anyway because we reuse allocations".
> The new code adds recycling of allocations, which I somehow expect
> in connection with static allocations where possible.  You're right
> that using strbuf_release() would match the original code more
> strictly.

I don't mind recycling allocations like this. It's just that I think in this case it makes the code actively more confusing, because we don't actually expect any of these allocations to be recycled, do we? We fork+exec for each daemon connection, which should receive exactly one `host=` parameter.

That being said, I don't mind it too much if the recycling stays here.

> But this block is a no-op anyway because it's the first thing we do
> to these (initially empty) variables.  That's not immediately
> obvious and should be addressed in a separate patch.

Ah, yeah, just reading the diff I thought this was cleanup, not initialization.

-Peff