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

> On 28.01.2018 07:40, Eric Sunshine wrote:
>> 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.
>
> I didn’t want to sound like I was just scratching my own itch ;) I hope
> this option is useful for other use-cases as well?

>> I understand that Junio suggested the name --send-log-to=, but I
>> wonder if the more concise --log= would be an possibility.
>
> --log sounds to me like it could also indicate *what* to log (e. g. “log
> verbosely” or “don’t log client IPs”). But perhaps --log-to= ?

>>>                 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
>>
>
> I’m not sure if that’s worth the extra complication… some existing
> options behave the same way already, e. g. in `git rebase --stat
> --quiet`, the `--stat` is ignored.

>>> +                       }
>>> +                       else if (!strcmp(v, "stderr")) {
>>
>> Style: cuddle 'else' with the braces: } else if (...) {
>>
>
> Is that a general rule? I couldn’t find anything about it in
> CodingGuidelines and daemon.c seemed to use both styles about evenly, so
> I wasn’t sure what to use.