Re: [PATCH] daemon: add systemd support

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 17, 2015, 19:58 UTC
Message-ID: <CAPig+cSOc8Qde1oxq2V+1m4gjnMHaa_mKXO4n21V6zQox9=6qg@mail.gmail.com>
In-Reply-To: <1431830650-111684-1-git-send-email-shawn@churchofgit.com>

On Sat, May 16, 2015 at 10:44 PM, Shawn Landden <shawn@churchofgit.com> wrote:

Show 10 quoted lines

> daemon: add systemd support
>
> git-daemon's --systemd mode allows git-daemon to be connect-activated
> on one or more addresses or ports. Unlike --inetd[1], git-daemon is
> not spawned for every connection.
>
> [1]which systemd is compatible with using its Accept=yes mode
>
> Signed-off-by: Shawn Landden <shawn@churchofgit.com>
> ---

For convenience of other reviewers, this is v8. Links to all versions:

v8 (2015-05-17): http://thread.gmane.org/gmane.comp.version-control.git/269205

v7.1 (2015-04-08): http://thread.gmane.org/gmane.comp.version-control.git/266632/focus=266969

v7 (2015-04-07): http://thread.gmane.org/gmane.comp.version-control.git/266926

v6 (2015-04-07): http://thread.gmane.org/gmane.comp.version-control.git/266895

v5 (2015-04-04): http://thread.gmane.org/gmane.comp.version-control.git/266759

v4 (2015-04-03): http://git.661346.n2.nabble.com/RFCv4-PATCH-daemon-add-systemd-support-td7628351.html

v3 (2015-04-03): http://git.661346.n2.nabble.com/v3RFC-systemd-socket-activation-support-td7628336.html

v2 (2015-04-02): http://thread.gmane.org/gmane.comp.version-control.git/266646

v1.1 (2015-04-02): http://thread.gmane.org/gmane.comp.version-control.git/266632

v1 (2015-04-02): http://thread.gmane.org/gmane.comp.version-control.git/266628

Below are some additional comments beyond what Junio already mentioned in his review...

Show 10 quoted lines

> diff --git a/Makefile b/Makefile
> index 36655d5..54986a0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -997,6 +1000,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)

It is highly unusual to place such an expensive check directly in Makefile (or even config.mak.uname) where it will penalize everyone (who hasn't disabled systemd) each time 'make' is invoked. This sort of expensive detection is typically only done by the configure script.

Show 30 quoted lines

> +               BASIC_CFLAGS += -DHAVE_SYSTEMD
> +               EXTLIBS += -lsystemd
> +       endif
> +endif
> diff --git a/daemon.c b/daemon.c
> index d3d3e43..42e1441 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1166,12 +1174,40 @@ static struct credentials *prepare_credentials(const char *user_name,
>  }
>  #endif
>
> +#ifdef HAVE_SYSTEMD
> +static int enumerate_sockets(struct socketlist *socklist, struct string_list *listen_addr, int listen_port, int systemd_mode)
> +{
> +       if (systemd_mode) {
> +               int i, n;
> +
> +               n = sd_listen_fds(0);
> +               if (n <= 0)
> +                       die("--systemd mode specified and no file descriptors recieved");
> +               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)
> +               socksetup(listen_addr, listen_port, socklist);
> +
> +       return 0;

What is the significance of the return value of enumerate_sockets()? It's unconditionally 0, even if socksetup() was never invoked, and isn't checked by the caller.

Show 19 quoted lines

> +}
> +#else
> +static int enumerate_sockets(struct socketlist *socklist, struct string_list *listen_addr, int listen_port, int systemd_mode)
> +{
> +       socksetup(listen_addr, listen_port, socklist);
> +
> +       return 0;
> +}
> +#endif
> @@ -1340,8 +1382,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

This #if is unnecessary since 'systemd_mode' will never become true (1) when HAVE_SYSTEMD is not defined, thus neither of the two following 'if' conditionals will trigger anyhow.

Show 10 quoted lines

> +       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");
> --
> 2.2.1.209.g41e5f3a