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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Jan 28, 2018 at 5:58 PM, Lucas Werkmeister
<mail@lucaswerkmeister.de> wrote:
> 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?

If the reader does not know that --inetd implies --syslog, then

    This makes it possible to use --inetd while still logging to
    standard error.

leads to a "Huh?" moment since it is not self-contained. Had it said

    Add new option --send-log-to=(stderr|syslog|none) which
    allows the implied --syslog by --inetd to be overridden.

it would have provided enough information to understand the purpose of
the patch at a glance. Talking about the systemd race-condition first
would also have explained the patch's purpose, and since the proposed
solution is general (not specific to your use-case), scratching an
itch is not a point against it.

Anyhow, it's not that big of a deal, but it did give me a bit of a
pause when reading the first paragraph since it's customary on this
project to start by explaining the problem.

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

Perhaps we can take into consideration precedent by other (non-Git)
daemon-like commands when naming this option. (None come to my mind
immediately, but I'm sure they're out there.)

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

I took "last one wins" into consideration when writing the above but
was not convinced that it applies to this case since --inetd and
--send-log-to= have no obvious relation to one another (unlike, say,
--verbose and --quiet or other similar combinations). Unless one reads
the documentation very closely, output ending up in syslog despite
"--send-log-to=stderr --inetd" is just way too counterintuitive and
may well lead to bug reports later on. Therefore, doing the additional
work now to stave off such bug reports is likely worthwhile.

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

It's not stated explicitly in CodingGuidelines, but there is one
example of cuddling 'else' with braces in the section talking about
braces and 'if' statements.