Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)
On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister <mail@lucaswerkmeister.de> wrote:
Show 15 quoted lines
> This makes it possible to use --inetd while still logging to standard
> error. --syslog is retained as an alias for --send-log-to=syslog. A mode
> to disable logging explicitly is also provided.
>
> The combination of --inetd with --send-log-to=stderr is useful, for
> instance, when running `git daemon` as an instanced systemd service
> (with associated socket unit). In this case, log messages sent via
> syslog are received by the journal daemon, but run the risk of being
> processed at a time when the `git daemon` process has already exited
> (especially if the process was very short-lived, e.g. due to client
> error), so that the journal daemon can no longer read its cgroup and
> attach the message to the correct systemd unit (see systemd/systemd#2913
> [1]). Logging to stderr instead can solve this problem, because systemd
> can connect stderr directly to the journal daemon, which then already
> knows which unit is associated with this stream.
The purpose of this patch would be easier to fathom if the problem was presented first (systemd race condition), followed by the solution (ability to log to stderr even when using --inetd), followed finally by incidental notes ("--syslog is retained as an alias..." and ability to disable logging).
Not sure, though, if it's worth a re-roll.
Show 11 quoted lines
> Signed-off-by: Lucas Werkmeister <mail@lucaswerkmeister.de>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> Notes:
> This was originally “daemon: add --no-syslog to undo implicit
> --syslog”, but Junio pointed out that combining --no-syslog with
> --detach isn’t especially useful and suggested --send-log-to=
> instead. Is Helped-by: the right credit for this or should it be
> something else?
Helped-by: is fine, though typically your Signed-off-by: would be last.
I understand that Junio suggested the name --send-log-to=, but I wonder if the more concise --log= would be an possibility.
More below...
Show 7 quoted lines
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> @@ -110,8 +111,26 @@ OPTIONS
> +--send-log-to=<destination>::
> + Send log messages to the specified destination.
> + Note that this option does not imply --verbose,
> + thus by default only error conditions will be logged.
> + The <destination> defaults to `stderr`, and must be one of:
Perhaps also update the documentation of --inetd to mention that its implied --syslog can be overridden by --send-log-to=.
Show 15 quoted lines
> diff --git a/daemon.c b/daemon.c
> @@ -74,11 +79,14 @@ static const char *get_ip_address(struct hostinfo *hi)
>
> static void logreport(int priority, const char *err, va_list params)
> {
> - if (log_syslog) {
> + switch (log_destination) {
> + case LOG_TO_SYSLOG: {
> char buf[1024];
> vsnprintf(buf, sizeof(buf), err, params);
> syslog(priority, "%s", buf);
> - } else {
> + break;
> + }
> + case LOG_TO_STDERR: {There aren't many instances of:
case FOO: {in the code-base, but those that exist don't use braces around cases which don't need it, so perhaps drop it from the STDERR and NONE cases. (Probably not worth a re-roll, though.)
Show 13 quoted lines
> /*
> * Since stderr is set to buffered mode, the
> * logging of different processes will not overlap
> @@ -88,6 +96,11 @@ static void logreport(int priority, const char *err, va_list params)
> vfprintf(stderr, err, params);
> fputc('\n', stderr);
> fflush(stderr);
> + break;
> + }
> + case LOG_TO_NONE: {
> + break;
> + }
> }Consecutive lines with braces at the same indentation level is rather odd (but see previous comment).
Show 8 quoted lines
> }
>
> @@ -1289,7 +1302,7 @@ int cmd_main(int argc, const char **argv)
> }
> if (!strcmp(arg, "--inetd")) {
> inetd_mode = 1;
> - log_syslog = 1;
> + log_destination = LOG_TO_SYSLOG;Hmm, so an invocation "--inetd --send-log-to=stderr" works as expected, but "--send-log-to=stderr --inetd" doesn't; output goes to syslog despite the explicit request for stderr. Counterintuitive. This should probably distinguish between 'log_destination' being unset and set explicitly; if unset, then, and only then, have --inetd imply syslog. Perhaps something like this:
static enum log_destination {
LOG_TO_UNSET = -1
LOG_TO_NONE,
LOG_TO_STDERR,
LOG_TO_SYSLOG,
} log_destination = LOG_TO_UNSET; if (!strcmp(arg, "--inetd")) {
inetd_mode = 1;
if (log_destination == LOG_TO_UNSET)
log_destination = LOG_TO_SYSLOG;
...
}
...
if (log_destination == LOG_TO_UNSET)
log_destination = LOG_TO_STDERRShow 9 quoted lines
> continue;
> }
> @@ -1297,9 +1310,25 @@ int cmd_main(int argc, const char **argv)
> + if (skip_prefix(arg, "--send-log-to=", &v)) {
> + if (!strcmp(v, "syslog")) {
> + log_destination = LOG_TO_SYSLOG;
> + continue;
> + }
> + else if (!strcmp(v, "stderr")) {Style: cuddle 'else' with the braces: } else if (...) {Show 9 quoted lines
> + log_destination = LOG_TO_STDERR;
> + continue;
> + }
> + else if (!strcmp(v, "none")) {
> + log_destination = LOG_TO_NONE;
> + continue;
> + }
> + else
> + die("Unknown log destination %s", v);Even though there is a mix of capitalized and lowercase-only error messages in daemon.c, newly written code avoids capitalization so perhaps downcase "unknown".
Lucas Werkmeister
