git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] systemd socket activation support

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Apr 2, 2015, 05:59 UTC
Message-ID
<CAPig+cQVLGAYKNJf2dZGpnZbU-GBzeVKpQG48cpFtX8uYZ_LPQ@mail.gmail.com>
In-Reply-To
<1427937796-10060-1-git-send-email-shawn@churchofgit.com>
On Wed, Apr 1, 2015 at 9:23 PM, Shawn Landden <shawn@churchofgit.com> wrote:
> From: Shawn Landden <shawnlandden@gmail.com>
>
> [PATCH] systemd socket activation support

This patch feels like an RFC rather than a properly fleshed-out submission. If so, indicate such in the subject. Also, mention the area you're touching, followed by a colon, followed by the summary of the change:

    [PATCH/RFC] daemon: add systemd support

The commit message may be a bit lacking. You might want to explain why this is desirable, perhaps mentioning that this complements existing inetd support, and (for the uninitiated) how it differs from inetd support (possibly citing documentation). It might also be a good idea to mention that sd-daemon.[ch] are foreign imports (possibly citing the source).

> Signed-off-by: Shawn Landden <shawn@churchofgit.com>
The Signed-off-by: email address differs from your From: address.
Show 6 quoted lines
> ---
>  daemon.c           |  38 ++++++++++++---
>  git-daemon.service |   6 +++
>  git-daemon.socket  |   9 ++++
>  sd-daemon.c        | 132 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sd-daemon.h        |  91 ++++++++++++++++++++++++++++++++++++

Necessary Documentation/git-daemon.txt changes are missing. Makefile changes are likely missing.

More below.
Show 9 quoted lines
> diff --git a/daemon.c b/daemon.c
> index 9ee2187..56b3cd4 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -5,6 +5,8 @@
>  #include "strbuf.h"
>  #include "string-list.h"
>
> +#include "sd-daemon.c"

This is kind of weird. Why this rather than the more typical approach of including sd-daemon.h here, compiling sd-daemon.c separately, and then combining them at link time? If there really is a good reason for this arrangement, it's probably worthwhile explaining it in the commit message.

Show 21 quoted lines
>  #ifndef HOST_NAME_MAX
>  #define HOST_NAME_MAX 256
>  #endif
> @@ -29,6 +31,7 @@ static const char daemon_usage[] =
>  "           [--access-hook=<path>]\n"
>  "           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\n"
>  "                      [--detach] [--user=<user> [--group=<group>]]\n"
> +"           [--systemd]\n"
>  "           [<directory>...]";
>
>  /* List of acceptable pathname prefixes */
> @@ -1176,11 +1179,21 @@ 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 };
> +       int i;
> +       int n;

These variables are used only within the 'if (systemd_mode)' scope, thus can be declared there.

More below.
Show 40 quoted lines
> -       socksetup(listen_addr, listen_port, &socklist);
> +       if (systemd_mode) {
> +               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 || !systemd_mode)
> +               socksetup(listen_addr, listen_port, &socklist);
>         if (socklist.nr == 0)
>                 die("unable to allocate any listen sockets on port %u",
>                     listen_port);
> @@ -1196,7 +1209,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 +1344,10 @@ int main(int argc, char **argv)
>                         informative_errors = 0;
>                         continue;
>                 }
> +               if (!strcmp(arg, "--systemd")) {
> +                       systemd_mode = 1;
> +                       continue;
> +               }
>                 if (!strcmp(arg, "--")) {
>                         ok_paths = &argv[i+1];
>                         break;
> @@ -1349,14 +1366,23 @@ 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))
I can't see how a variable named 'systemd-mode' would ever have compiled.
Show 14 quoted lines
> +               die("--detach, --user and --group are incompatible with --inetd and --systemd");
> +
> +       if (systemd_mode && inetd_mode)
> +               die("--inetd is incompatible with --systemd");
>
>         if (inetd_mode && (listen_port || (listen_addr.nr > 0)))
>                 die("--listen= and --port= are incompatible with --inetd");
>         else if (listen_port == 0)
>                 listen_port = DEFAULT_GIT_PORT;
>
> +       if (systemd_mode) {
> +               i = sd_listen_fds(0);
> +               if (i <= 0)
> +                       die("--systemd passed and not running from systemd or no file descriptors passed");
Perhaps rephrase as:
    --systemd requested but not invoked from systemd or file
descriptors not specified
Show 24 quoted lines
> +       }
> +
>         if (group_name && !user_name)
>                 die("--group supplied without --user");
>
> @@ -1395,5 +1421,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);
>  }
> diff --git a/git-daemon.service b/git-daemon.service
> new file mode 100644
> index 0000000..78c662e
> --- /dev/null
> +++ b/git-daemon.service
> @@ -0,0 +1,6 @@
> +[Unit]
> +Description=Git Daemon
> +
> +[Service]
> +ExecStart=/usr/lib/git-core/git-daemon --systemd --base-path=/var/lib /var/lib/git
> +User=gitdaemon

If the intention is that this file should get installed somewhere so that the admin can copy it or link to it, then you will want to augment the Makefile to do so. Likewise, the hardcoded paths (/usr/lib/git-core and /var/lib/git) probably require munging based upon the installation location.

An alternative would be to take a hint from what was done for inetd support, and instead add this to Documentation/git-daemon.txt as an example configuration file.

Show 14 quoted lines
> diff --git a/git-daemon.socket b/git-daemon.socket
> new file mode 100644
> index 0000000..b3dd981
> --- /dev/null
> +++ b/git-daemon.socket
> @@ -0,0 +1,9 @@
> +[Unit]
> +Description=Git Daemon socket
> +
> +[Socket]
> +ListenStream=9418
> +
> +[Install]
> +WantedBy=sockets.target

Ditto regarding Makefile support or adding this as an example to Documentation/git-daemon.txt.

Previous: Shawn LanddenNext: Junio C Hamano
Message 2 of 4 in “systemd socket activation support”
  1. systemd socket activation supportShawn Landden, Apr 2, 2015
  2. Eric SunshineApr 2, 2015
  3. Junio C HamanoApr 2, 2015
  4. Shawn LanddenApr 2, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.