[v6 PATCH] daemon: add systemd support

[v6 PATCH] daemon: add systemd support

[Shawn Landden]

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>
---
Respond to review by Eric Sunshine here:
http://marc.info/?l=git&m=142836529908207&w=2

I formatted the example files to mimic `systemctl show` output, but what was suggested
is better.
 Documentation/git-daemon.txt | 47 +++++++++++++++++++++++++++++++++++++++-----
 Makefile                     | 10 ++++++++++
 daemon.c                     | 46 +++++++++++++++++++++++++++++++++++++------
 3 files changed, 92 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index a69b361..3a7a0b1 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -19,7 +19,8 @@ SYNOPSIS
 	     [--access-hook=<path>] [--[no-]informative-errors]
 	     [--inetd |
 	      [--listen=<host_or_ipaddr>] [--port=<n>]
-	      [--user=<user> [--group=<group>]]]
+	      [--systemd |
+	       [--user=<user> [--group=<group>]]]
 	     [<directory>...]
 
 DESCRIPTION
@@ -81,8 +82,8 @@ OPTIONS
 
 --inetd::
 	Have the server run as an inetd service. Implies --syslog.
-	Incompatible with --detach, --port, --listen, --user and --group
-	options.
+	Incompatible with --systemd, --detach, --port, --listen, --user and
+	--group options.
 
 --listen=<host_or_ipaddr>::
 	Listen on a specific IP address or hostname.  IP addresses can
@@ -146,8 +147,8 @@ OPTIONS
 	the option are given to `getpwnam(3)` and `getgrnam(3)`
 	and numeric IDs are not supported.
 +
-Giving these options is an error when used with `--inetd`; use
-the facility of inet daemon to achieve the same before spawning
+Giving these options is an error when used with `--inetd` or `--systemd`; use
+the facility of systemd or the inet daemon to achieve the same before spawning
 'git daemon' if needed.
 +
 Like many programs that switch user id, the daemon does not reset
@@ -180,6 +181,14 @@ Git configuration files in that directory are readable by `<user>`.
 	errors are not enabled, all errors report "access denied" to the
 	client. The default is --no-informative-errors.
 
+--systemd::
+	For running git-daemon under systemd(1) which will pass
+	an open connection. This is similar to --inetd, except
+	that more than one address/port can be listened to at once
+	both through systemd and through --listen, and git-daemon doesn't get
+	invoked for every connection. For more details see systemd.socket(5).
+	Incompatible with --inetd, --detach, --user and --group options.
+
 --access-hook=<path>::
 	Every time a client connects, first run an external command
 	specified by the <path> with service name (e.g. "upload-pack"),
@@ -304,7 +313,35 @@ selectively enable/disable services per repository::
 		uploadpack = false
 		uploadarch = true
 ----------------------------------------------------------------
++
+systemd configuration example::
+Example systemd configuration files, typically placed in `/etc/systemd/system`.
++
+`git-daemon.socket`
++
+----------------------------------------------------------------
+# /etc/systemd/system/git-daemon.socket
+[Unit]
+Description=Git Daemon socket
+
+[Socket]
+ListenStream=9418
+
+[Install]
+WantedBy=sockets.target
+----------------------------------------------------------------
++
+`git-daemon.service`
++
+----------------------------------------------------------------
+[Unit]
+Description=Git Daemon
 
+[Service]
+ExecStart=/usr/lib/git-core/git-daemon --systemd --reuseaddr --base-path=/var/lib /var/lib/git
+User=git-daemon
+StandardError=null
+----------------------------------------------------------------
 
 ENVIRONMENT
 -----------
diff --git a/Makefile b/Makefile
index 5f3987f..415ac21 100644
--- a/Makefile
+++ b/Makefile
@@ -42,6 +42,9 @@ all::
 # Define NO_EXPAT if you do not have expat installed.  git-http-push is
 # not built, and you cannot push using http:// and https:// transports (dumb).
 #
+# Define NO_SYSTEMD to prevent systemd socket activation support from being
+# built into git-daemon.
+#
 # Define EXPATDIR=/foo/bar if your expat header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
 #
@@ -995,6 +998,13 @@ ifeq ($(uname_S),Darwin)
 	PTHREAD_LIBS =
 endif
 
+ifndef NO_SYSTEMD
+	ifeq ($(shell echo "\#include <systemd/sd-daemon.h>" | $(CC) -E - -o /dev/null 2>/dev/null && echo y),y)
+		BASIC_CFLAGS += -DHAVE_SYSTEMD
+		EXTLIBS += -lsystemd
+	endif
+endif
+
 ifndef CC_LD_DYNPATH
 	ifdef NO_R_TO_GCC_LINKER
 		# Some gcc does not accept and pass -R to the linker to specify
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 };
 
-	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);
 	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);
 }
-- 
2.2.1.209.g41e5f3a

Re: [v6 PATCH] daemon: add systemd support

[Eric Sunshine]

On Mon, Apr 6, 2015 at 10:03 PM, Shawn Landden <shawn@churchofgit.com> wrote:
> 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.

The commit message is much better than previous versions, although, it
fails to parse cleanly and does not seem to have been proof-read. The
first sentence still feels as if its unrelated to the rest of the
commit message, however, prefixing it with "although" helps. Perhaps
something like this:

    Although "git-daemon --inetd" works with systemd, a proper
    "git-daemon --systemd" mode has the advantage of ... listening
    on multiple interfaces, ..., does not require spawning a new
    git-daemon instance for each connection, ...

Fill in the "..." with whatever other benefits a --systemd mode might
provide (if any).

> Signed-off-by: Shawn Landden <shawn@churchofgit.com>
> ---
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index a69b361..3a7a0b1 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -304,7 +313,35 @@ selectively enable/disable services per repository::
>                 uploadpack = false
>                 uploadarch = true
>  ----------------------------------------------------------------
> ++

As mentioned in the last couple reviews, you should replace the above
"+" line with a blank line since the "systemd configuration example"
item is not a continuation of the preceding "selectively
enable/disable services per repository" item.

> +systemd configuration example::
> +Example systemd configuration files, typically placed in `/etc/systemd/system`.
> ++
> +`git-daemon.socket`
> ++
> +----------------------------------------------------------------
> +# /etc/systemd/system/git-daemon.socket

This comment line merely repeats the filename outside of the verbatim
block, thus can be dropped.

> +[Unit]
> +Description=Git Daemon socket
> +
> +[Socket]
> +ListenStream=9418
> +
> +[Install]
> +WantedBy=sockets.target
> +----------------------------------------------------------------
> ++
> +`git-daemon.service`
> ++
> +----------------------------------------------------------------
> +[Unit]
> +Description=Git Daemon
>
> +[Service]
> +ExecStart=/usr/lib/git-core/git-daemon --systemd --reuseaddr --base-path=/var/lib /var/lib/git
> +User=git-daemon
> +StandardError=null
> +----------------------------------------------------------------
>
>  ENVIRONMENT
>  -----------

Re: [v6 PATCH] daemon: add systemd support

[Ramsay Jones]

On 07/04/15 03:03, Shawn Landden wrote:
> 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]

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

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

>  	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);
>  }
> 

Re: [v6 PATCH] daemon: add systemd support

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> On 07/04/15 03:03, Shawn Landden wrote:
>> 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]
>
>> 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 ...
>
>> -	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, ... ;-)


Yes, I'd really prefer to avoid #if/#else#endif in the main
codeflow.

It is vastly prefereable, if you can arrange so, to have a set of
helper functions

#if USE_SYSTEMD
static void enumerate_sockets(struct socklist *slist)
{
    ... /* do systemd specific enumeration */
}
#else
static void enumerate_sockets(struct socklist *slist)
{
    ... /* something else */
}
#endif

and then just call that helper from the main codeline.

	enumerate_sockets(&socklist);
        socksetup(...);

without #ifdef/#else/#endif