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