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