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

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

quoted

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