On Sat, Mar 07, 2015 at 01:54:12AM +0100, René Scharfe wrote: > >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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html