On Sun, Jan 28, 2018 at 5:58 PM, Lucas Werkmeister
<mail@xxxxxxxxxxxxxxxxxxx> wrote:
> On 28.01.2018 07:40, Eric Sunshine wrote:
>> On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
>> <mail@xxxxxxxxxxxxxxxxxxx> 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.
