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

Am 06.03.2015 um 22:06 schrieb Jeff King:

On Fri, Mar 06, 2015 at 09:57:22AM +0100, René Scharfe wrote:

quoted

  				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...

quoted

-	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é