Re: [v6 PATCH] daemon: add systemd support

Ramsay Jones [off-list ref] writes:

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

quoted

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

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

quoted

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

[snip]

quoted

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

quoted

-	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