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

[Jeff King <peff@peff.net>]

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

> Convert hostname, canon_hostname, ip_address and tcp_port to strbuf.
> This allows to get rid of the helpers strbuf_addstr_or_null() and STRARG
> because a strbuf always represents a valid (initially empty) string.
> sanitize_client() becomes unused and is removed as well.

Makes sense. I had a feeling that we might have cared about NULL versus
the empty string somewhere, but I did not see it in the patch below, so
I think it is fine.

> -static char *sanitize_client(const char *in)
> -{
> -	struct strbuf out = STRBUF_INIT;
> -	sanitize_client_strbuf(&out, in);
> -	return strbuf_detach(&out, NULL);
> -}

Not a big deal, but do we want to rename sanitize_client_strbuf to
sanitize_client? It only had the unwieldy name to distinguish it from
this one.

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

-Peff