From: René Scharfe <l.s.r@web.de>
Date: Sat, 07 Mar 2015 00:54:12 GMT
Subject: Re: [PATCH] daemon: use strbuf for hostname info
Message-ID: <54FA4C34.6080702@web.de>
In-Reply-To: <20150306210627.GA24267@peff.net>

Am 06.03.2015 um 22:06 schrieb Jeff King:
> On Fri, Mar 06, 2015 at 09:57:22AM +0100, René Scharfe wrote:
>>   				if (port) {
>> -					free(tcp_port);
>> -					tcp_port = sanitize_client(port);
>> +					strbuf_reset(&tcp_port);
>> +					sanitize_client_strbuf(&tcp_port, port);
>
> The equivalent of free() is strbuf_release(). I think it is reasonable
> to strbuf_reset here, since we are about to write into it again anyway
> (though I doubt it happens much in practice, since that would imply
> multiple `host=` segments sent by the client). But later...
>
>> -	free(hostname);
>> -	free(canon_hostname);
>> -	free(ip_address);
>> -	free(tcp_port);
>> -	hostname = canon_hostname = ip_address = tcp_port = NULL;
>> +	strbuf_reset(&hostname);
>> +	strbuf_reset(&canon_hostname);
>> +	strbuf_reset(&ip_address);
>> +	strbuf_reset(&tcp_port);
>
> 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.

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.

René