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

[René Scharfe]

Am 07.03.2015 um 02:08 schrieb Jeff King:

On Sat, Mar 07, 2015 at 01:20:22AM +0100, René Scharfe wrote:


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.


A patch would look like this.  The result is shorter, but no win in
terms of vertical space (number of lines).


IMHO this is an improvement, though whether it is enough to merit the
code churn I dunno. So I'm in favor, but don't mind dropping it if
others disagree.


I don't think the change justifies a separate patch, but we can squash 
it in. :)


René

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[René Scharfe]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Sat, Mar 07, 2015 at 01:54:12AM +0100, René Scharfe wrote:

 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.

I don't mind recycling allocations like this. It's just that I think in
this case it makes the code actively more confusing, because we don't
actually expect any of these allocations to be recycled, do we? We
fork+exec for each daemon connection, which should receive exactly one
`host=` parameter.

That being said, I don't mind it too much if the recycling stays here.

 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.

Ah, yeah, just reading the diff I thought this was cleanup, not
initialization.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[René Scharfe]

Am 06.03.2015 um 22:06 schrieb Jeff King:
 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.

A patch would look like this.  The result is shorter, but no win in
terms of vertical space (number of lines).

-- 8 --
Subject: daemon: drop _strbuf suffix of sanitize and canonicalize functions

Now that only the strbuf variants of the functions remain, remove the
_strbuf part from their names.

Suggested-by: Jeff King p...@peff.net
Signed-off-by: Rene Scharfe l@web.de
---
 daemon.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/daemon.c b/daemon.c
index c04315e..0412f8c 100644
--- a/daemon.c
+++ b/daemon.c
@@ -534,7 +534,7 @@ static void parse_host_and_port(char *hostport, char **host,
  * trailing and leading dots, which means that the client cannot escape
  * our base path via .. traversal.
  */
-static void sanitize_client_strbuf(struct strbuf *out, const char *in)
+static void sanitize_client(struct strbuf *out, const char *in)
 {
for (; *in; in++) {
if (*in == '/')
@@ -549,12 +549,12 @@ static void sanitize_client_strbuf(struct strbuf *out, 
const char *in)
 }
 
 /*
- * Like sanitize_client_strbuf, but we also perform any canonicalization
+ * Like sanitize_client, but we also perform any canonicalization
  * to make life easier on the admin.
  */
-static void canonicalize_client_strbuf(struct strbuf *out, const char *in)
+static void canonicalize_client(struct strbuf *out, const char *in)
 {
-   sanitize_client_strbuf(out, in);
+   sanitize_client(out, in);
strbuf_tolower(out);
 }
 
@@ -579,10 +579,10 @@ static void parse_host_arg(char *extra_args, int buflen)
parse_host_and_port(val, host, port);
if (port) {
strbuf_reset(tcp_port);
-   sanitize_client_strbuf(tcp_port, port);
+   sanitize_client(tcp_port, port);
}
strbuf_reset(hostname);
-   canonicalize_client_strbuf(hostname, host);
+   canonicalize_client(hostname, host);
hostname_lookup_done = 0;
}
 
@@ -620,8 +620,8 @@ static void lookup_hostname(void)
 
strbuf_reset(canon_hostname);
if (ai-ai_canonname)
-   sanitize_client_strbuf(canon_hostname,
-  ai-ai_canonname);
+   sanitize_client(canon_hostname,
+   ai-ai_canonname);
else
strbuf_addbuf(canon_hostname, ip_address);
 
@@ -645,7 +645,7 @@ static void lookup_hostname(void)
  addrbuf, sizeof(addrbuf));
 
strbuf_reset(canon_hostname);
-   sanitize_client_strbuf(canon_hostname, hent-h_name);
+   sanitize_client(canon_hostname, hent-h_name);
strbuf_reset(ip_address);
strbuf_addstr(ip_address, addrbuf);
}
-- 
2.3.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Sat, Mar 07, 2015 at 01:20:22AM +0100, René Scharfe wrote:

  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.
 
 A patch would look like this.  The result is shorter, but no win in
 terms of vertical space (number of lines).

IMHO this is an improvement, though whether it is enough to merit the
code churn I dunno. So I'm in favor, but don't mind dropping it if
others disagree.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] daemon: use strbuf for hostname info

[René Scharfe]

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.

Signed-off-by: Rene Scharfe l@web.de
---
 daemon.c | 98 +++-
 1 file changed, 41 insertions(+), 57 deletions(-)

diff --git a/daemon.c b/daemon.c
index c3edd96..c04315e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -56,10 +56,10 @@ static const char *user_path;
 static unsigned int timeout;
 static unsigned int init_timeout;
 
-static char *hostname;
-static char *canon_hostname;
-static char *ip_address;
-static char *tcp_port;
+static struct strbuf hostname = STRBUF_INIT;
+static struct strbuf canon_hostname = STRBUF_INIT;
+static struct strbuf ip_address = STRBUF_INIT;
+static struct strbuf tcp_port = STRBUF_INIT;
 
 static int hostname_lookup_done;
 
@@ -68,13 +68,13 @@ static void lookup_hostname(void);
 static const char *get_canon_hostname(void)
 {
lookup_hostname();
-   return canon_hostname;
+   return canon_hostname.buf;
 }
 
 static const char *get_ip_address(void)
 {
lookup_hostname();
-   return ip_address;
+   return ip_address.buf;
 }
 
 static void logreport(int priority, const char *err, va_list params)
@@ -122,12 +122,6 @@ static void NORETURN daemon_die(const char *err, va_list 
params)
exit(1);
 }
 
-static void strbuf_addstr_or_null(struct strbuf *sb, const char *s)
-{
-   if (s)
-   strbuf_addstr(sb, s);
-}
-
 struct expand_path_context {
const char *directory;
 };
@@ -138,22 +132,22 @@ static size_t expand_path(struct strbuf *sb, const char 
*placeholder, void *ctx)
 
switch (placeholder[0]) {
case 'H':
-   strbuf_addstr_or_null(sb, hostname);
+   strbuf_addbuf(sb, hostname);
return 1;
case 'C':
if (placeholder[1] == 'H') {
-   strbuf_addstr_or_null(sb, get_canon_hostname());
+   strbuf_addstr(sb, get_canon_hostname());
return 2;
}
break;
case 'I':
if (placeholder[1] == 'P') {
-   strbuf_addstr_or_null(sb, get_ip_address());
+   strbuf_addstr(sb, get_ip_address());
return 2;
}
break;
case 'P':
-   strbuf_addstr_or_null(sb, tcp_port);
+   strbuf_addbuf(sb, tcp_port);
return 1;
case 'D':
strbuf_addstr(sb, context-directory);
@@ -301,16 +295,14 @@ static int run_access_hook(struct daemon_service 
*service, const char *dir, cons
char *eol;
int seen_errors = 0;
 
-#define STRARG(x) ((x) ? (x) : )
*arg++ = access_hook;
*arg++ = service-name;
*arg++ = path;
-   *arg++ = STRARG(hostname);
-   *arg++ = STRARG(get_canon_hostname());
-   *arg++ = STRARG(get_ip_address());
-   *arg++ = STRARG(tcp_port);
+   *arg++ = hostname.buf;
+   *arg++ = get_canon_hostname();
+   *arg++ = get_ip_address();
+   *arg++ = tcp_port.buf;
*arg = NULL;
-#undef STRARG
 
child.use_shell = 1;
child.argv = argv;
@@ -556,23 +548,14 @@ static void sanitize_client_strbuf(struct strbuf *out, 
const char *in)
strbuf_setlen(out, out-len - 1);
 }
 
-static char *sanitize_client(const char *in)
-{
-   struct strbuf out = STRBUF_INIT;
-   sanitize_client_strbuf(out, in);
-   return strbuf_detach(out, NULL);
-}
-
 /*
- * Like sanitize_client, but we also perform any canonicalization
+ * Like sanitize_client_strbuf, but we also perform any canonicalization
  * to make life easier on the admin.
  */
-static char *canonicalize_client(const char *in)
+static void canonicalize_client_strbuf(struct strbuf *out, const char *in)
 {
-   struct strbuf out = STRBUF_INIT;
-   sanitize_client_strbuf(out, in);
-   strbuf_tolower(out);
-   return strbuf_detach(out, NULL);
+   sanitize_client_strbuf(out, in);
+   strbuf_tolower(out);
 }
 
 /*
@@ -595,11 +578,11 @@ static void parse_host_arg(char *extra_args, int buflen)
char *port;
parse_host_and_port(val, host, port);
if (port) {
-   free(tcp_port);
-   tcp_port = sanitize_client(port);
+   strbuf_reset(tcp_port);
+   sanitize_client_strbuf(tcp_port, port);
}
-   free(hostname);
-   hostname = canonicalize_client(host);
+   strbuf_reset(hostname);
+ 

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

[Jeff King]

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html