{"thread":{"id":"38999","subject":"[RFCv5 PATCH] daemon: add systemd support","startedAt":"2015-04-04T03:22:20Z","lastAt":"2015-04-07T00:08:11Z","messageCount":2,"participants":["Shawn Landden","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"258975","messageId":"1428117740-32622-1-git-send-email-shawn@churchofgit.com","threadId":"38999","inReplyTo":null,"subject":"[RFCv5 PATCH] daemon: add systemd support","fromName":"Shawn Landden","fromEmail":"shawn@churchofgit.com","sentAt":"2015-04-04T03:22:20Z","receivedAt":"2015-04-04T03:22:20Z","isPatch":true,"sender":{"key":"shawn@churchofgit.com","avatar":"https://gravatar.com/avatar/66fa92115ac78085af2524ca39ccca362bd1ca7571a4b0b843a8a7017b210df2?d=mp&s=160"},"body":"systemd supports git-daemon's existing --inetd mode as well.\n\nSigned-off-by: Shawn Landden <shawn@churchofgit.com>\n---\n Documentation/git-daemon.txt | 41 +++++++++++++++++++++++++++++++++++-----\n Makefile                     | 10 ++++++++++\n daemon.c                     | 45 ++++++++++++++++++++++++++++++++++++++------\n 3 files changed, 85 insertions(+), 11 deletions(-)\n\nRespond to review in http://article.gmane.org/gmane.comp.version-control.git/266650\nI did not indent the example documents as that was for inetd, and that would break copy/paste.\n\nThese are all documentation changes, no functional differences. (Well, the example\ngained StandardError=null to match --inetd)\n\nv5: do not change whitespace of Makefile\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex a69b361..a273565 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -18,7 +18,7 @@ SYNOPSIS\n \t     [--allow-override=<service>] [--forbid-override=<service>]\n \t     [--access-hook=<path>] [--[no-]informative-errors]\n \t     [--inetd |\n-\t      [--listen=<host_or_ipaddr>] [--port=<n>]\n+\t      [--listen=<host_or_ipaddr>] [--port=<n>] [--systemd]\n \t      [--user=<user> [--group=<group>]]]\n \t     [<directory>...]\n \n@@ -81,8 +81,8 @@ OPTIONS\n \n --inetd::\n \tHave the server run as an inetd service. Implies --syslog.\n-\tIncompatible with --detach, --port, --listen, --user and --group\n-\toptions.\n+\tIncompatible with --systemd, --detach, --port, --listen, --user and\n+\t--group options.\n \n --listen=<host_or_ipaddr>::\n \tListen on a specific IP address or hostname.  IP addresses can\n@@ -146,8 +146,8 @@ OPTIONS\n \tthe option are given to `getpwnam(3)` and `getgrnam(3)`\n \tand numeric IDs are not supported.\n +\n-Giving these options is an error when used with `--inetd`; use\n-the facility of inet daemon to achieve the same before spawning\n+Giving these options is an error when used with `--inetd` or `--systemd`; use\n+the facility of systemd or the inet daemon to achieve the same before spawning\n 'git daemon' if needed.\n +\n Like many programs that switch user id, the daemon does not reset\n@@ -180,6 +180,14 @@ Git configuration files in that directory are readable by `<user>`.\n \terrors are not enabled, all errors report \"access denied\" to the\n \tclient. The default is --no-informative-errors.\n \n+--systemd::\n+\tFor running git-daemon under systemd(1) which will pass\n+\tan open connection. This is similar to --inetd, except\n+\tthat more than one address/port can be listened to at once\n+\tboth through systemd and through --listen, and git-daemon doesn't get\n+\tinvoked for every connection. For more details see systemd.socket(5).\n+\tIncompatible with --inetd, --detach, --user and --group options.\n+\n --access-hook=<path>::\n \tEvery time a client connects, first run an external command\n \tspecified by the <path> with service name (e.g. \"upload-pack\"),\n@@ -304,7 +312,30 @@ selectively enable/disable services per repository::\n \t\tuploadpack = false\n \t\tuploadarch = true\n ----------------------------------------------------------------\n++\n+\n+systemd configuration example::\n++\n+----------------------------------------------------------------\n+# /etc/systemd/system/git-daemon.socket\n+[Unit]\n+Description=Git Daemon socket\n \n+[Socket]\n+ListenStream=9418\n+\n+[Install]\n+WantedBy=sockets.target\n+\n+# /etc/systemd/system/git-daemon.service\n+[Unit]\n+Description=Git Daemon\n+\n+[Service]\n+ExecStart=/usr/lib/git-core/git-daemon --systemd --reuseaddr --base-path=/var/lib /var/lib/git\n+User=git-daemon\n+StandardError=null\n+----------------------------------------------------------------\n \n ENVIRONMENT\n -----------\ndiff --git a/Makefile b/Makefile\nindex 5f3987f..83f5d8e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -42,6 +42,9 @@ all::\n # Define NO_EXPAT if you do not have expat installed.  git-http-push is\n # not built, and you cannot push using http:// and https:// transports (dumb).\n #\n+# Define NO_SYSTEMD to prevent systemd socket activation support from being\n+# built into git-daemon.\n+#\n # Define EXPATDIR=/foo/bar if your expat header and library files are in\n # /foo/bar/include and /foo/bar/lib directories.\n #\n@@ -995,6 +998,13 @@ ifeq ($(uname_S),Darwin)\n \tPTHREAD_LIBS =\n endif\n \n+ifndef NO_SYSTEMD\n+\tifeq ($(shell echo \"\\#include <systemd/sd-daemon.h>\" | $(CC) -E - -o /dev/null && echo y),y)\n+\t\tBASIC_CFLAGS += -DHAVE_SYSTEMD\n+\t\tEXTLIBS += -lsystemd\n+\tendif\n+endif\n+\n ifndef CC_LD_DYNPATH\n \tifdef NO_R_TO_GCC_LINKER\n \t\t# Some gcc does not accept and pass -R to the linker to specify\ndiff --git a/daemon.c b/daemon.c\nindex 9ee2187..ad8a79a 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1,3 +1,7 @@\n+#ifdef HAVE_SYSTEMD\n+#  include <systemd/sd-daemon.h>\n+#endif\n+\n #include \"cache.h\"\n #include \"pkt-line.h\"\n #include \"exec_cmd.h\"\n@@ -29,6 +33,9 @@ static const char daemon_usage[] =\n \"           [--access-hook=<path>]\\n\"\n \"           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\\n\"\n \"                      [--detach] [--user=<user> [--group=<group>]]\\n\"\n+#ifdef HAVE_SYSTEMD\n+\"           [--systemd]\\n\"\n+#endif\n \"           [<directory>...]\";\n \n /* List of acceptable pathname prefixes */\n@@ -1176,11 +1183,23 @@ static void store_pid(const char *path)\n }\n \n static int serve(struct string_list *listen_addr, int listen_port,\n-    struct credentials *cred)\n+    struct credentials *cred, int systemd_mode)\n {\n \tstruct socketlist socklist = { NULL, 0, 0 };\n \n-\tsocksetup(listen_addr, listen_port, &socklist);\n+#ifdef HAVE_SYSTEMD\n+\tif (systemd_mode) {\n+\t\tint i, n;\n+\n+\t\tn = sd_listen_fds(0);\n+\t\tALLOC_GROW(socklist.list, socklist.nr + n, socklist.alloc);\n+\t\tfor (i = 0; i < n; i++)\n+\t\t\tsocklist.list[socklist.nr++] = SD_LISTEN_FDS_START + i;\n+\t}\n+\n+\tif (listen_addr->nr > 0 || !systemd_mode)\n+#endif\n+\t\tsocksetup(listen_addr, listen_port, &socklist);\n \tif (socklist.nr == 0)\n \t\tdie(\"unable to allocate any listen sockets on port %u\",\n \t\t    listen_port);\n@@ -1196,7 +1215,7 @@ int main(int argc, char **argv)\n {\n \tint listen_port = 0;\n \tstruct string_list listen_addr = STRING_LIST_INIT_NODUP;\n-\tint serve_mode = 0, inetd_mode = 0;\n+\tint serve_mode = 0, inetd_mode = 0, systemd_mode = 0;\n \tconst char *pid_file = NULL, *user_name = NULL, *group_name = NULL;\n \tint detach = 0;\n \tstruct credentials *cred = NULL;\n@@ -1331,6 +1350,12 @@ int main(int argc, char **argv)\n \t\t\tinformative_errors = 0;\n \t\t\tcontinue;\n \t\t}\n+#ifdef HAVE_SYSTEMD\n+\t\tif (!strcmp(arg, \"--systemd\")) {\n+\t\t\tsystemd_mode = 1;\n+\t\t\tcontinue;\n+\t\t}\n+#endif\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\tok_paths = &argv[i+1];\n \t\t\tbreak;\n@@ -1349,8 +1374,16 @@ int main(int argc, char **argv)\n \t\t/* avoid splitting a message in the middle */\n \t\tsetvbuf(stderr, NULL, _IOFBF, 4096);\n \n-\tif (inetd_mode && (detach || group_name || user_name))\n-\t\tdie(\"--detach, --user and --group are incompatible with --inetd\");\n+\tif ((inetd_mode || systemd_mode) && (detach || group_name || user_name))\n+\t\tdie(\"--detach, --user and --group are incompatible with --inetd and --systemd\");\n+\n+#ifdef HAVE_SYSTEMD\n+\tif (systemd_mode && inetd_mode)\n+\t\tdie(\"--inetd is incompatible with --systemd\");\n+\n+\tif (systemd_mode && !sd_booted())\n+\t\tdie(\"--systemd passed and not invoked from systemd\");\n+#endif\n \n \tif (inetd_mode && (listen_port || (listen_addr.nr > 0)))\n \t\tdie(\"--listen= and --port= are incompatible with --inetd\");\n@@ -1395,5 +1428,5 @@ int main(int argc, char **argv)\n \t\tcld_argv[i+1] = argv[i];\n \tcld_argv[argc+1] = NULL;\n \n-\treturn serve(&listen_addr, listen_port, cred);\n+\treturn serve(&listen_addr, listen_port, cred, systemd_mode);\n }\n-- \n2.2.1.209.g41e5f3a\n"},{"id":"259108","messageId":"CAPig+cTfj_j_rX6Bgz9UY8hYWzjCS4eSXa1DsS3FtMwP3+7i_g@mail.gmail.com","threadId":"38999","inReplyTo":"1428117740-32622-1-git-send-email-shawn@churchofgit.com","subject":"Re: [RFCv5 PATCH] daemon: add systemd support","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-07T00:08:11Z","receivedAt":"2015-04-07T00:08:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 3, 2015 at 11:22 PM, Shawn Landden <shawn@churchofgit.com> wrote:\n> [RFCv5 PATCH] daemon: add systemd support\n\nNow that you've included a documentation update and made the example\nsystemd configuration files more accessible, this feels like a more\nproperly fleshed-out submission (though the commit message is still\nquite lacking), so it's probably time to drop the \"RFC\" designation.\n\n> systemd supports git-daemon's existing --inetd mode as well.\n\nWhat is the intention of this sentence, and how is it relevant to the patch?\n\nAre you saying that git-daemon already works correctly with systemd\nwhen using the --inetd option? If so, then the commit message should\nexplain how the new --systemd option is superior to --inetd in order\nto justify the added complexity to both code and documentation. Some\nof what you wrote in the documentation update may be useful in the\ncommit message to help convince the reader that the patch is\nworthwhile.\n\n> Signed-off-by: Shawn Landden <shawn@churchofgit.com>\n> ---\n> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n> index a69b361..a273565 100644\n> --- a/Documentation/git-daemon.txt\n> +++ b/Documentation/git-daemon.txt\n> @@ -18,7 +18,7 @@ SYNOPSIS\n>              [--allow-override=<service>] [--forbid-override=<service>]\n>              [--access-hook=<path>] [--[no-]informative-errors]\n>              [--inetd |\n> -             [--listen=<host_or_ipaddr>] [--port=<n>]\n> +             [--listen=<host_or_ipaddr>] [--port=<n>] [--systemd]\n>               [--user=<user> [--group=<group>]]]\n\nThe documentation (below) for the --systemd option says that it is\nincompatible with --user= and --group=, so shouldn't this be:\n\n    [--inetd |\n        [--listen=<host_or_ipaddr>] [--port=<n>]\n        [(--systemd | --user=<user> [--group=<group>])]]\n\nor something?\n\nMore below.\n\n>              [<directory>...]\n>\n> @@ -81,8 +81,8 @@ OPTIONS\n>\n>  --inetd::\n>         Have the server run as an inetd service. Implies --syslog.\n> -       Incompatible with --detach, --port, --listen, --user and --group\n> -       options.\n> +       Incompatible with --systemd, --detach, --port, --listen, --user and\n> +       --group options.\n>\n>  --listen=<host_or_ipaddr>::\n>         Listen on a specific IP address or hostname.  IP addresses can\n> @@ -146,8 +146,8 @@ OPTIONS\n>         the option are given to `getpwnam(3)` and `getgrnam(3)`\n>         and numeric IDs are not supported.\n>  +\n> -Giving these options is an error when used with `--inetd`; use\n> -the facility of inet daemon to achieve the same before spawning\n> +Giving these options is an error when used with `--inetd` or `--systemd`; use\n> +the facility of systemd or the inet daemon to achieve the same before spawning\n>  'git daemon' if needed.\n>  +\n>  Like many programs that switch user id, the daemon does not reset\n> @@ -180,6 +180,14 @@ Git configuration files in that directory are readable by `<user>`.\n>         errors are not enabled, all errors report \"access denied\" to the\n>         client. The default is --no-informative-errors.\n>\n> +--systemd::\n> +       For running git-daemon under systemd(1) which will pass\n> +       an open connection. This is similar to --inetd, except\n> +       that more than one address/port can be listened to at once\n> +       both through systemd and through --listen, and git-daemon doesn't get\n> +       invoked for every connection. For more details see systemd.socket(5).\n> +       Incompatible with --inetd, --detach, --user and --group options.\n> +\n>  --access-hook=<path>::\n>         Every time a client connects, first run an external command\n>         specified by the <path> with service name (e.g. \"upload-pack\"),\n> @@ -304,7 +312,30 @@ selectively enable/disable services per repository::\n>                 uploadpack = false\n>                 uploadarch = true\n>  ----------------------------------------------------------------\n> ++\n\nDrop this \"+\" line.\n\n> +\n> +systemd configuration example::\n> ++\n> +----------------------------------------------------------------\n> +# /etc/systemd/system/git-daemon.socket\n> +[Unit]\n> +Description=Git Daemon socket\n>\n> +[Socket]\n> +ListenStream=9418\n> +\n> +[Install]\n> +WantedBy=sockets.target\n> +\n> +# /etc/systemd/system/git-daemon.service\n> +[Unit]\n> +Description=Git Daemon\n> +\n> +[Service]\n> +ExecStart=/usr/lib/git-core/git-daemon --systemd --reuseaddr --base-path=/var/lib /var/lib/git\n> +User=git-daemon\n> +StandardError=null\n> +----------------------------------------------------------------\n\nUnfortunately, plopping both of these example files into a single\nverbatim block makes it difficult to see that there are indeed two\nfiles being shown. Someone could easily overlook the second file after\nseeing the initial \"# /etc/systemd/system/git-daemon.socket\" comment,\nand merely copy/paste the entire block into that one file. It would be\nbetter to split this into two verbatim blocks, one for each file,\nwhile moving the filename outside the block, and including a bit more\nexplanation.\n\nFor instance:\n\n    systemd configuration example::\n        Example systemd configuration files, typically placed in\n        `/etc/systemd/system`.\n    +\n    `git-daemon.socket`\n    +\n    ----------------------------------------------------------------\n    [Unit]\n    Description=Git Daemon socket\n    ...\n    ----------------------------------------------------------------\n    +\n    `git-daemon.service`\n    +\n    ----------------------------------------------------------------\n    [Unit]\n    Description=Git Daemon\n    ...\n    ----------------------------------------------------------------\n\nAll the pluses and back-ticks in the above illustration are intentional.\n\nMore below.\n\n>  ENVIRONMENT\n>  -----------\n> diff --git a/Makefile b/Makefile\n> index 5f3987f..83f5d8e 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -42,6 +42,9 @@ all::\n>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is\n>  # not built, and you cannot push using http:// and https:// transports (dumb).\n>  #\n> +# Define NO_SYSTEMD to prevent systemd socket activation support from being\n> +# built into git-daemon.\n> +#\n>  # Define EXPATDIR=/foo/bar if your expat header and library files are in\n>  # /foo/bar/include and /foo/bar/lib directories.\n>  #\n> @@ -995,6 +998,13 @@ ifeq ($(uname_S),Darwin)\n>         PTHREAD_LIBS =\n>  endif\n>\n> +ifndef NO_SYSTEMD\n> +       ifeq ($(shell echo \"\\#include <systemd/sd-daemon.h>\" | $(CC) -E - -o /dev/null && echo y),y)\n\nUnfortunately, this spews scary \"<stdin>:1:10: fatal error:\n'systemd/sd-daemon.h' file not found\" errors at build-time on\nplatforms which lack that header. You might want to redirect stderr to\nsuppress the complaint. Perhaps like this:\n\n    $(shell echo \"...\" | $(CC) -E - -o /dev/null 2>/dev/null && ...)\n\nMore below.\n\n> +               BASIC_CFLAGS += -DHAVE_SYSTEMD\n> +               EXTLIBS += -lsystemd\n> +       endif\n> +endif\n> +\n>  ifndef CC_LD_DYNPATH\n>         ifdef NO_R_TO_GCC_LINKER\n>                 # Some gcc does not accept and pass -R to the linker to specify\n> diff --git a/daemon.c b/daemon.c\n> index 9ee2187..ad8a79a 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1,3 +1,7 @@\n> +#ifdef HAVE_SYSTEMD\n> +#  include <systemd/sd-daemon.h>\n> +#endif\n> +\n>  #include \"cache.h\"\n>  #include \"pkt-line.h\"\n>  #include \"exec_cmd.h\"\n> @@ -29,6 +33,9 @@ static const char daemon_usage[] =\n>  \"           [--access-hook=<path>]\\n\"\n>  \"           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\\n\"\n>  \"                      [--detach] [--user=<user> [--group=<group>]]\\n\"\n> +#ifdef HAVE_SYSTEMD\n> +\"           [--systemd]\\n\"\n> +#endif\n\nThis differs from the usage in the documentation where --systemd is\ngrouped with --listen and --port, etc. as an alternative to --inetd.\n\n>  \"           [<directory>...]\";\n>\n>  /* List of acceptable pathname prefixes */\n> @@ -1176,11 +1183,23 @@ static void store_pid(const char *path)\n>  }\n>\n>  static int serve(struct string_list *listen_addr, int listen_port,\n> -    struct credentials *cred)\n> +    struct credentials *cred, int systemd_mode)\n>  {\n>         struct socketlist socklist = { NULL, 0, 0 };\n>\n> -       socksetup(listen_addr, listen_port, &socklist);\n> +#ifdef HAVE_SYSTEMD\n> +       if (systemd_mode) {\n> +               int i, n;\n> +\n> +               n = sd_listen_fds(0);\n> +               ALLOC_GROW(socklist.list, socklist.nr + n, socklist.alloc);\n> +               for (i = 0; i < n; i++)\n> +                       socklist.list[socklist.nr++] = SD_LISTEN_FDS_START + i;\n> +       }\n> +\n> +       if (listen_addr->nr > 0 || !systemd_mode)\n> +#endif\n> +               socksetup(listen_addr, listen_port, &socklist);\n>         if (socklist.nr == 0)\n>                 die(\"unable to allocate any listen sockets on port %u\",\n>                     listen_port);\n> @@ -1196,7 +1215,7 @@ int main(int argc, char **argv)\n>  {\n>         int listen_port = 0;\n>         struct string_list listen_addr = STRING_LIST_INIT_NODUP;\n> -       int serve_mode = 0, inetd_mode = 0;\n> +       int serve_mode = 0, inetd_mode = 0, systemd_mode = 0;\n>         const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;\n>         int detach = 0;\n>         struct credentials *cred = NULL;\n> @@ -1331,6 +1350,12 @@ int main(int argc, char **argv)\n>                         informative_errors = 0;\n>                         continue;\n>                 }\n> +#ifdef HAVE_SYSTEMD\n> +               if (!strcmp(arg, \"--systemd\")) {\n> +                       systemd_mode = 1;\n> +                       continue;\n> +               }\n> +#endif\n>                 if (!strcmp(arg, \"--\")) {\n>                         ok_paths = &argv[i+1];\n>                         break;\n> @@ -1349,8 +1374,16 @@ int main(int argc, char **argv)\n>                 /* avoid splitting a message in the middle */\n>                 setvbuf(stderr, NULL, _IOFBF, 4096);\n>\n> -       if (inetd_mode && (detach || group_name || user_name))\n> -               die(\"--detach, --user and --group are incompatible with --inetd\");\n> +       if ((inetd_mode || systemd_mode) && (detach || group_name || user_name))\n> +               die(\"--detach, --user and --group are incompatible with --inetd and --systemd\");\n> +\n> +#ifdef HAVE_SYSTEMD\n> +       if (systemd_mode && inetd_mode)\n> +               die(\"--inetd is incompatible with --systemd\");\n> +\n> +       if (systemd_mode && !sd_booted())\n> +               die(\"--systemd passed and not invoked from systemd\");\n> +#endif\n>\n>         if (inetd_mode && (listen_port || (listen_addr.nr > 0)))\n>                 die(\"--listen= and --port= are incompatible with --inetd\");\n> @@ -1395,5 +1428,5 @@ int main(int argc, char **argv)\n>                 cld_argv[i+1] = argv[i];\n>         cld_argv[argc+1] = NULL;\n>\n> -       return serve(&listen_addr, listen_port, cred);\n> +       return serve(&listen_addr, listen_port, cred, systemd_mode);\n>  }\n> --\n> 2.2.1.209.g41e5f3a\n"}]}