Re: [RFC 2] systemd socket activation support
On Thu, Apr 2, 2015 at 12:15 PM, Shawn Landden sh...@churchofgit.com wrote: [RFC 2] systemd socket activation support Rephrase this as: [PATCH/RFC v2] daemon: add systemd support systemd support git-daemon's --inetd mode as well. Unable to make sense of this sentence (fragment). A better commit message would explain the purpose of the change, its justification, and any important issues which readers need to know to understand and review the patch. See my previous review[1] for some ideas. [1]: http://article.gmane.org/gmane.comp.version-control.git/266631 v2: actually test... It is indeed a good idea to explain what changed between versions of the patch, however, this isn't telling us much. To ease the task of reviewers, explain changes between versions with sufficient detail so that readers don't need to guess. Also, it is very helpful to provide a link to the previous version of the patch and its discussion since this is a high-traffic list, and reviewers won't necessarily remember the details of the previous version. Also, you don't generally want this sort of commentary recorded in the permanent project history, so it doesn't belong in the commit message proper. Instead, place it below the --- line following your sign-off, just above the diffstat. Signed-off-by: Shawn Landden sh...@churchofgit.com --- This is where you should place commentary which you don't want in the permanent project history. diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt index a69b361..898e01f 100644 --- a/Documentation/git-daemon.txt +++ b/Documentation/git-daemon.txt @@ -20,6 +20,7 @@ SYNOPSIS [--inetd | [--listen=host_or_ipaddr] [--port=n] [--user=user [--group=group]]] + [--systemd] [directory...] DESCRIPTION @@ -190,6 +191,12 @@ Git configuration files in that directory are readable by `user`. exiting with a zero status). It can also look at the $REMOTE_ADDR and $REMOTE_PORT environment variables to learn about the requestor when making this decision. +--systemd:: + For running git-daemon under systemd(1) which will pass + an open connection. This is similar to --inetd, except + that more than one address/port can be listened to at once + both through systemd and through --listen, and git-daemon doesn't get + invoked for every connection. For more details see systemd.socket(5). The documentation for the --inetd option mentions other options with which it is incompatible. This new documentation probably ought to do the same. Moreover, you should update the --inetd documentation to mention that it is incompatible with --systemd. Unfortunately, you plopped this new text right in the middle of the the description of --access-hook (which is continued by the + line and the left-justified paragraph below). If you format the documentation and look at the before and after output, you will see the problem. + The external command can optionally write a single line to its standard output to be sent to the requestor as an error message when @@ -304,7 +311,25 @@ selectively enable/disable services per repository:: uploadpack = false uploadarch = true Unfortunately, the new example you've added (below) doesn't format properly, and results in Asciidoc warnings. To ensure correctness, follow the other examples more closely, and try formatting it yourself. ++ This + line incorrectly ties your new example to the preceding selectively enable/disable services per repository item. Just insert a blank line here rather than a + line. +systemd configuration example: This item must end with ::, not just :. + Instead of a blank line, this should be a + line in order to tie the example (below) to the systemd configuration example item. +# /etc/systemd/system/git-daemon.socket +[Unit] +Description=Git Daemon socket + +[Socket] +ListenStream=9418 + +[Install] +WantedBy=sockets.target +# /etc/systemd/system/git-daemon.service +[Unit] +Description=Git Daemon +[Service] +ExecStart=/usr/lib/git-core/git-daemon --systemd --reuseaddr --base-path=/var/lib /var/lib/git +User=gitdaemon The example block must be delimited by -- lines to be formatted correctly as literal text. (Both -- lines should have the same number of dashes in order to satisfy all versions of Asciidoc). Also, indent the text as other examples in git-daemon.txt. More below. ENVIRONMENT --- diff --git a/daemon.c b/daemon.c index 9ee2187..e809a4c 100644 --- a/daemon.c +++ b/daemon.c @@ -4,6 +4,7 @@ #include run-command.h #include strbuf.h #include string-list.h +#include sd-daemon.h #ifndef HOST_NAME_MAX #define HOST_NAME_MAX 256 @@ -29,6 +30,7 @@ static const char daemon_usage[] = [--access-hook=path]\n
[RFC 2] systemd socket activation support
systemd support git-daemon's --inetd mode as well.
v2: actually test...
Signed-off-by: Shawn Landden sh...@churchofgit.com
---
Documentation/git-daemon.txt | 25 +++
Makefile | 1 +
daemon.c | 35 --
sd-daemon.c | 152 +++
sd-daemon.h | 104 +
5 files changed, 311 insertions(+), 6 deletions(-)
create mode 100644 sd-daemon.c
create mode 100644 sd-daemon.h
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index a69b361..898e01f 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
[--inetd |
[--listen=host_or_ipaddr] [--port=n]
[--user=user [--group=group]]]
+ [--systemd]
[directory...]
DESCRIPTION
@@ -190,6 +191,12 @@ Git configuration files in that directory are readable by
`user`.
exiting with a zero status). It can also look at the $REMOTE_ADDR
and $REMOTE_PORT environment variables to learn about the
requestor when making this decision.
+--systemd::
+ For running git-daemon under systemd(1) which will pass
+ an open connection. This is similar to --inetd, except
+ that more than one address/port can be listened to at once
+ both through systemd and through --listen, and git-daemon doesn't get
+ invoked for every connection. For more details see systemd.socket(5).
+
The external command can optionally write a single line to its
standard output to be sent to the requestor as an error message when
@@ -304,7 +311,25 @@ selectively enable/disable services per repository::
uploadpack = false
uploadarch = true
++
+systemd configuration example:
+
+# /etc/systemd/system/git-daemon.socket
+[Unit]
+Description=Git Daemon socket
+
+[Socket]
+ListenStream=9418
+
+[Install]
+WantedBy=sockets.target
+# /etc/systemd/system/git-daemon.service
+[Unit]
+Description=Git Daemon
+[Service]
+ExecStart=/usr/lib/git-core/git-daemon --systemd --reuseaddr
--base-path=/var/lib /var/lib/git
+User=gitdaemon
ENVIRONMENT
---
diff --git a/Makefile b/Makefile
index 5f3987f..4a813b9 100644
--- a/Makefile
+++ b/Makefile
@@ -765,6 +765,7 @@ LIB_OBJS += rerere.o
LIB_OBJS += resolve-undo.o
LIB_OBJS += revision.o
LIB_OBJS += run-command.o
+LIB_OBJS += sd-daemon.o
LIB_OBJS += send-pack.o
LIB_OBJS += sequencer.o
LIB_OBJS += server-info.o
diff --git a/daemon.c b/daemon.c
index 9ee2187..e809a4c 100644
--- a/daemon.c
+++ b/daemon.c
@@ -4,6 +4,7 @@
#include run-command.h
#include strbuf.h
#include string-list.h
+#include sd-daemon.h
#ifndef HOST_NAME_MAX
#define HOST_NAME_MAX 256
@@ -29,6 +30,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 +1178,22 @@ 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 };
- socksetup(listen_addr, listen_port, socklist);
+ if (systemd_mode) {
+ int i;
+ int 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)
+ 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,20 @@ int main(int argc, char **argv)
/* avoid splitting a message in
