{"thread":{"id":"39020","subject":"[v6 PATCH] daemon: add systemd support","startedAt":"2015-04-07T02:03:26Z","lastAt":"2015-04-09T03:51:38Z","messageCount":4,"participants":["Shawn Landden","Eric Sunshine","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"259110","messageId":"1428372206-120089-1-git-send-email-shawn@churchofgit.com","threadId":"39020","inReplyTo":null,"subject":"[v6 PATCH] daemon: add systemd support","fromName":"Shawn Landden","fromEmail":"shawn@churchofgit.com","sentAt":"2015-04-07T02:03:26Z","receivedAt":"2015-04-07T02:03:26Z","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--systemd allows git-daemon has the advantage of allowing one git-daemon\nto listen to multiple interfaces as well as the system one(s),\nand more allow git-daemon to not be spawned on every connection.\n\nSigned-off-by: Shawn Landden <shawn@churchofgit.com>\n---\nRespond to review by Eric Sunshine here:\nhttp://marc.info/?l=git&m=142836529908207&w=2\n\nI formatted the example files to mimic `systemctl show` output, but what was suggested\nis better.\n Documentation/git-daemon.txt | 47 +++++++++++++++++++++++++++++++++++++++-----\n Makefile                     | 10 ++++++++++\n daemon.c                     | 46 +++++++++++++++++++++++++++++++++++++------\n 3 files changed, 92 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex a69b361..3a7a0b1 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -19,7 +19,8 @@ SYNOPSIS\n \t     [--access-hook=<path>] [--[no-]informative-errors]\n \t     [--inetd |\n \t      [--listen=<host_or_ipaddr>] [--port=<n>]\n-\t      [--user=<user> [--group=<group>]]]\n+\t      [--systemd |\n+\t       [--user=<user> [--group=<group>]]]\n \t     [<directory>...]\n \n DESCRIPTION\n@@ -81,8 +82,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 +147,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 +181,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 +313,35 @@ selectively enable/disable services per repository::\n \t\tuploadpack = false\n \t\tuploadarch = true\n ----------------------------------------------------------------\n++\n+systemd configuration example::\n+Example systemd configuration files, typically placed in `/etc/systemd/system`.\n++\n+`git-daemon.socket`\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++\n+`git-daemon.service`\n++\n+----------------------------------------------------------------\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..415ac21 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 2>/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..9880858 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@@ -28,7 +32,11 @@ static const char daemon_usage[] =\n \"           [--(enable|disable|allow-override|forbid-override)=<service>]\\n\"\n \"           [--access-hook=<path>]\\n\"\n \"           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\\n\"\n+#ifdef HAVE_SYSTEMD\n+\"                      [--systemd | [--detach] [--user=<user> [--group=<group>]]]\\n\" /* exactly 80 characters */\n+#else\n \"                      [--detach] [--user=<user> [--group=<group>]]\\n\"\n+#endif\n \"           [<directory>...]\";\n \n /* List of acceptable pathname prefixes */\n@@ -1176,11 +1184,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 +1216,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 +1351,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 +1375,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 +1429,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":"259112","messageId":"CAPig+cQn30sTAYeBGrH_WNxN5Hsme+uqqUtHsm7bTNvVZ7XBiA@mail.gmail.com","threadId":"39020","inReplyTo":"1428372206-120089-1-git-send-email-shawn@churchofgit.com","subject":"Re: [v6 PATCH] daemon: add systemd support","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-07T04:42:14Z","receivedAt":"2015-04-07T04:42:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 6, 2015 at 10:03 PM, Shawn Landden <shawn@churchofgit.com> wrote:\n> systemd supports git-daemon's existing --inetd mode as well.\n> --systemd allows git-daemon has the advantage of allowing one git-daemon\n> to listen to multiple interfaces as well as the system one(s),\n> and more allow git-daemon to not be spawned on every connection.\n\nThe commit message is much better than previous versions, although, it\nfails to parse cleanly and does not seem to have been proof-read. The\nfirst sentence still feels as if its unrelated to the rest of the\ncommit message, however, prefixing it with \"although\" helps. Perhaps\nsomething like this:\n\n    Although \"git-daemon --inetd\" works with systemd, a proper\n    \"git-daemon --systemd\" mode has the advantage of ... listening\n    on multiple interfaces, ..., does not require spawning a new\n    git-daemon instance for each connection, ...\n\nFill in the \"...\" with whatever other benefits a --systemd mode might\nprovide (if any).\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..3a7a0b1 100644\n> --- a/Documentation/git-daemon.txt\n> +++ b/Documentation/git-daemon.txt\n> @@ -304,7 +313,35 @@ selectively enable/disable services per repository::\n>                 uploadpack = false\n>                 uploadarch = true\n>  ----------------------------------------------------------------\n> ++\n\nAs mentioned in the last couple reviews, you should replace the above\n\"+\" line with a blank line since the \"systemd configuration example\"\nitem is not a continuation of the preceding \"selectively\nenable/disable services per repository\" item.\n\n> +systemd configuration example::\n> +Example systemd configuration files, typically placed in `/etc/systemd/system`.\n> ++\n> +`git-daemon.socket`\n> ++\n> +----------------------------------------------------------------\n> +# /etc/systemd/system/git-daemon.socket\n\nThis comment line merely repeats the filename outside of the verbatim\nblock, thus can be dropped.\n\n> +[Unit]\n> +Description=Git Daemon socket\n> +\n> +[Socket]\n> +ListenStream=9418\n> +\n> +[Install]\n> +WantedBy=sockets.target\n> +----------------------------------------------------------------\n> ++\n> +`git-daemon.service`\n> ++\n> +----------------------------------------------------------------\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>  -----------\n"},{"id":"259123","messageId":"5523B933.8080302@ramsay1.demon.co.uk","threadId":"39020","inReplyTo":"1428372206-120089-1-git-send-email-shawn@churchofgit.com","subject":"Re: [v6 PATCH] daemon: add systemd support","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2015-04-07T11:02:11Z","receivedAt":"2015-04-07T11:02:11Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 07/04/15 03:03, Shawn Landden wrote:\n> systemd supports git-daemon's existing --inetd mode as well.\n> --systemd allows git-daemon has the advantage of allowing one git-daemon\n> to listen to multiple interfaces as well as the system one(s),\n> and more allow git-daemon to not be spawned on every connection.\n> \n> Signed-off-by: Shawn Landden <shawn@churchofgit.com>\n> ---\n\nI have not been following this patch review, but I just happened to\nnotice something while skimming the patch as this email floated by ...\n\n> Respond to review by Eric Sunshine here:\n> http://marc.info/?l=git&m=142836529908207&w=2\n> \n\n[snip]\n\n> diff --git a/daemon.c b/daemon.c\n> index 9ee2187..9880858 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> @@ -28,7 +32,11 @@ static const char daemon_usage[] =\n>  \"           [--(enable|disable|allow-override|forbid-override)=<service>]\\n\"\n>  \"           [--access-hook=<path>]\\n\"\n>  \"           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\\n\"\n> +#ifdef HAVE_SYSTEMD\n> +\"                      [--systemd | [--detach] [--user=<user> [--group=<group>]]]\\n\" /* exactly 80 characters */\n> +#else\n>  \"                      [--detach] [--user=<user> [--group=<group>]]\\n\"\n> +#endif\n>  \"           [<directory>...]\";\n>  \n>  /* List of acceptable pathname prefixes */\n> @@ -1176,11 +1184,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\n... this hunk splits a statement in two ...\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\n... here. I have not considered any possible subtlety in the code, but simply\nconsidering the patch text, I think the following may be easier to read:\n\n    #ifdef HAVE_SYSTEMD\n    \tif (systemd_mode) {\n    \t...\n    \t}\n\n    \tif (listen_addr->nr > 0 || !systemd_mode)\n    \t\tsocksetup(listen_addr, listen_port, &socklist);\n    #else\n    \tsocksetup(listen_addr, listen_port, &socklist);\n    #endif\n\nOr, maybe:\n\n    #if !defined(HAVE_SYSTEMD)\n    \tsocksetup(listen_addr, listen_port, &socklist);\n    #else\n    \t...\n    #endif\n\nOr, ... ;-)\n\nATB,\nRamsay Jones\n\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 +1216,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 +1351,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 +1375,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 +1429,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> \n"},{"id":"259195","messageId":"xmqqr3ruarhx.fsf@gitster.dls.corp.google.com","threadId":"39020","inReplyTo":"5523B933.8080302@ramsay1.demon.co.uk","subject":"Re: [v6 PATCH] daemon: add systemd support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-09T03:51:38Z","receivedAt":"2015-04-09T03:51:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> On 07/04/15 03:03, Shawn Landden wrote:\n>> systemd supports git-daemon's existing --inetd mode as well.\n>> --systemd allows git-daemon has the advantage of allowing one git-daemon\n>> to listen to multiple interfaces as well as the system one(s),\n>> and more allow git-daemon to not be spawned on every connection.\n>> \n>> Signed-off-by: Shawn Landden <shawn@churchofgit.com>\n>> ---\n>\n> I have not been following this patch review, but I just happened to\n> notice something while skimming the patch as this email floated by ...\n>\n>> Respond to review by Eric Sunshine here:\n>> http://marc.info/?l=git&m=142836529908207&w=2\n>> \n>\n> [snip]\n>\n>> diff --git a/daemon.c b/daemon.c\n>> index 9ee2187..9880858 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>> @@ -28,7 +32,11 @@ static const char daemon_usage[] =\n>>  \"           [--(enable|disable|allow-override|forbid-override)=<service>]\\n\"\n>>  \"           [--access-hook=<path>]\\n\"\n>>  \"           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\\n\"\n>> +#ifdef HAVE_SYSTEMD\n>> +\"                      [--systemd | [--detach] [--user=<user> [--group=<group>]]]\\n\" /* exactly 80 characters */\n>> +#else\n>>  \"                      [--detach] [--user=<user> [--group=<group>]]\\n\"\n>> +#endif\n>>  \"           [<directory>...]\";\n>>  \n>>  /* List of acceptable pathname prefixes */\n>> @@ -1176,11 +1184,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>\n> ... this hunk splits a statement in two ...\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>\n> ... here. I have not considered any possible subtlety in the code, but simply\n> considering the patch text, I think the following may be easier to read:\n>\n>     #ifdef HAVE_SYSTEMD\n>     \tif (systemd_mode) {\n>     \t...\n>     \t}\n>\n>     \tif (listen_addr->nr > 0 || !systemd_mode)\n>     \t\tsocksetup(listen_addr, listen_port, &socklist);\n>     #else\n>     \tsocksetup(listen_addr, listen_port, &socklist);\n>     #endif\n>\n> Or, maybe:\n>\n>     #if !defined(HAVE_SYSTEMD)\n>     \tsocksetup(listen_addr, listen_port, &socklist);\n>     #else\n>     \t...\n>     #endif\n>\n> Or, ... ;-)\n\n\nYes, I'd really prefer to avoid #if/#else#endif in the main\ncodeflow.\n\nIt is vastly prefereable, if you can arrange so, to have a set of\nhelper functions\n\n#if USE_SYSTEMD\nstatic void enumerate_sockets(struct socklist *slist)\n{\n    ... /* do systemd specific enumeration */\n}\n#else\nstatic void enumerate_sockets(struct socklist *slist)\n{\n    ... /* something else */\n}\n#endif\n\nand then just call that helper from the main codeline.\n\n\tenumerate_sockets(&socklist);\n        socksetup(...);\n\nwithout #ifdef/#else/#endif\n"}]}