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

From: Jeff King <peff@peff.net>
Date: Mar 6, 2015, 21:06 UTC
Message-ID: <20150306210627.GA24267@peff.net>
In-Reply-To: <54F96BF2.5000504@web.de>

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.

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 9 quoted lines

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