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"
> " [--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;
Minor: If you examine other parts of daemon.c, you will find that
these declarations are normally placed together on a single line, so
you could collapse this to:
int i, n = sd_listen_fds(0);
or:
int i, n;
n = ...;
in order make better use of precious vertical screen real-estate.
More below.
> +
> + 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;
The --inetd switch implies --syslog. Should --systemd do the same by
setting "log_syslog = 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 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");
> +
> + 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 && !sd_booted())
> + die("--systemd passed and not invoked from systemd");
> +
> if (group_name && !user_name)
> die("--group supplied without --user");
Note shown in this patch, but --inetd redirects stderr to /dev/null.
Should --systemd be doing so as well?
> @@ -1395,5 +1418,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);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
