Re: [v6 PATCH] daemon: add systemd support

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Apr 7, 2015, 11:02 UTC
Message-ID: <5523B933.8080302@ramsay1.demon.co.uk>
In-Reply-To: <1428372206-120089-1-git-send-email-shawn@churchofgit.com>

On 07/04/15 03:03, Shawn Landden wrote:

Show 7 quoted lines

> systemd supports git-daemon's existing --inetd mode as well.
> --systemd allows git-daemon has the advantage of allowing one git-daemon
> to listen to multiple interfaces as well as the system one(s),
> and more allow git-daemon to not be spawned on every connection.
> 
> Signed-off-by: Shawn Landden <shawn@churchofgit.com>
> ---

I have not been following this patch review, but I just happened to notice something while skimming the patch as this email floated by ...

> Respond to review by Eric Sunshine here:
> http://marc.info/?l=git&m=142836529908207&w=2
>

[snip]

Show 33 quoted lines

> diff --git a/daemon.c b/daemon.c
> index 9ee2187..9880858 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1,3 +1,7 @@
> +#ifdef HAVE_SYSTEMD
> +#  include <systemd/sd-daemon.h>
> +#endif
> +
>  #include "cache.h"
>  #include "pkt-line.h"
>  #include "exec_cmd.h"
> @@ -28,7 +32,11 @@ static const char daemon_usage[] =
>  "           [--(enable|disable|allow-override|forbid-override)=<service>]\n"
>  "           [--access-hook=<path>]\n"
>  "           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\n"
> +#ifdef HAVE_SYSTEMD
> +"                      [--systemd | [--detach] [--user=<user> [--group=<group>]]]\n" /* exactly 80 characters */
> +#else
>  "                      [--detach] [--user=<user> [--group=<group>]]\n"
> +#endif
>  "           [<directory>...]";
>  
>  /* List of acceptable pathname prefixes */
> @@ -1176,11 +1184,23 @@ static void store_pid(const char *path)
>  }
>  
>  static int serve(struct string_list *listen_addr, int listen_port,
> -    struct credentials *cred)
> +    struct credentials *cred, int systemd_mode)
>  {
>  	struct socketlist socklist = { NULL, 0, 0 };
>

... this hunk splits a statement in two ...

Show 14 quoted lines

> -	socksetup(listen_addr, listen_port, &socklist);
> +#ifdef HAVE_SYSTEMD
> +	if (systemd_mode) {
> +		int i, n;
> +
> +		n = sd_listen_fds(0);
> +		ALLOC_GROW(socklist.list, socklist.nr + n, socklist.alloc);
> +		for (i = 0; i < n; i++)
> +			socklist.list[socklist.nr++] = SD_LISTEN_FDS_START + i;
> +	}
> +
> +	if (listen_addr->nr > 0 || !systemd_mode)
> +#endif
> +		socksetup(listen_addr, listen_port, &socklist);

... here. I have not considered any possible subtlety in the code, but simply considering the patch text, I think the following may be easier to read:

    #ifdef HAVE_SYSTEMD
    	if (systemd_mode) {
    	...
    	}

    	if (listen_addr->nr > 0 || !systemd_mode)
    		socksetup(listen_addr, listen_port, &socklist);
    #else
    	socksetup(listen_addr, listen_port, &socklist);
    #endif

Or, maybe:

    #if !defined(HAVE_SYSTEMD)
    	socksetup(listen_addr, listen_port, &socklist);
    #else
    	...
    #endif

Or, ... ;-)

ATB, Ramsay Jones

Show 52 quoted lines

>  	if (socklist.nr == 0)
>  		die("unable to allocate any listen sockets on port %u",
>  		    listen_port);
> @@ -1196,7 +1216,7 @@ int main(int argc, char **argv)
>  {
>  	int listen_port = 0;
>  	struct string_list listen_addr = STRING_LIST_INIT_NODUP;
> -	int serve_mode = 0, inetd_mode = 0;
> +	int serve_mode = 0, inetd_mode = 0, systemd_mode = 0;
>  	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
>  	int detach = 0;
>  	struct credentials *cred = NULL;
> @@ -1331,6 +1351,12 @@ int main(int argc, char **argv)
>  			informative_errors = 0;
>  			continue;
>  		}
> +#ifdef HAVE_SYSTEMD
> +		if (!strcmp(arg, "--systemd")) {
> +			systemd_mode = 1;
> +			continue;
> +		}
> +#endif
>  		if (!strcmp(arg, "--")) {
>  			ok_paths = &argv[i+1];
>  			break;
> @@ -1349,8 +1375,16 @@ int main(int argc, char **argv)
>  		/* avoid splitting a message in the middle */
>  		setvbuf(stderr, NULL, _IOFBF, 4096);
>  
> -	if (inetd_mode && (detach || group_name || user_name))
> -		die("--detach, --user and --group are incompatible with --inetd");
> +	if ((inetd_mode || systemd_mode) && (detach || group_name || user_name))
> +		die("--detach, --user and --group are incompatible with --inetd and --systemd");
> +
> +#ifdef HAVE_SYSTEMD
> +	if (systemd_mode && inetd_mode)
> +		die("--inetd is incompatible with --systemd");
> +
> +	if (systemd_mode && !sd_booted())
> +		die("--systemd passed and not invoked from systemd");
> +#endif
>  
>  	if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
>  		die("--listen= and --port= are incompatible with --inetd");
> @@ -1395,5 +1429,5 @@ int main(int argc, char **argv)
>  		cld_argv[i+1] = argv[i];
>  	cld_argv[argc+1] = NULL;
>  
> -	return serve(&listen_addr, listen_port, cred);
> +	return serve(&listen_addr, listen_port, cred, systemd_mode);
>  }
>