Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
<mail@lucaswerkmeister.de> wrote:
> 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.

> 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...

> 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=.

> 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.)

>                 /*
>                  * 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).

>  }
>
> @@ -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_STDERR

>                         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 (...) {

> +                               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".