Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Lucas Werkmeister]

On 24.01.2018 19:33, Junio C Hamano wrote:
> Lucas Werkmeister  writes:
> 
>>> Moreover, --detach completely dissociates the process from the
>>> original set of standard file descriptors by first closing them and
>>> then connecting it to "/dev/null", so it will be nonsense to use this
>>> new option with it.
>>
>> Ah, I wasn’t aware of that – so with --detach, --no-syslog would be
>> better described as “disables all logging” rather than “log to stderr
>> instead”. IMHO it would still make sense to have the --no-syslog option
>> (then mainly for use with --inetd) as long as its interaction with
>> --inetd is properly documented.
> 
> Because "--detach --no-syslog" is a roundabout way to ask for
> sending the log to _nowhere_, I actually would say that "nonsense"
> is a bit too strong a word for the combination of your thing with
> "--detach".
> 
> It might make more sense to introduce a new "--send-log-to="
> option, where the destination can be one of: syslog, stderr, none.
> 
> The you can make the current "--syslog" option a synonym to
> "--send-log-to=syslog".  The internal variable log_syslog would
> probably become
> 
>   enum log_destination { 
>   LOG_TO_NONE = -1,
>   LOG_TO_STDERR = 0,
>   LOG_TO_SYSLOG = 1,
>   } log_destination;
> 
> and wherever the current code assigns 1 to log_syslog, you would be
> setting it LOG_TO_SYSLOG.
> 
> Then those who want no log can express that wish in a more direct
> way, i.e. "daemon --send-log-to=none", perhaps.
> 
> Such an approach leaves open room for future enhancement.  It is not
> too far-fetched to imagine something like:
> 
>   git daemon --send-log-to=/var/log/git-daemon.log
> 
> by introducing the fourth value to "enum log_destination"; perhaps
> the file is opened and connected to stderr to accept the logs,
> combined with a new feature that tells the daemon to close and
> reopen the log file when it receives a HUP or something like that.

Sounds interesting… do you think it would be worth it supporting
multiple destinations? Right now this could be implemented fairly easily
by making log_destination a bit field (and --syslog would then imply
--send-log-to=syslog --no-send-log-to=stderr or something like that). On
the other hand, that doesn’t allow for this nice trick of reusing the
stderr fd for a log file in case of future enhancement.

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Junio C Hamano]

Lucas Werkmeister  writes:

>> Moreover, --detach completely dissociates the process from the
>> original set of standard file descriptors by first closing them and
>> then connecting it to "/dev/null", so it will be nonsense to use this
>> new option with it.
>
> Ah, I wasn’t aware of that – so with --detach, --no-syslog would be
> better described as “disables all logging” rather than “log to stderr
> instead”. IMHO it would still make sense to have the --no-syslog option
> (then mainly for use with --inetd) as long as its interaction with
> --inetd is properly documented.

Because "--detach --no-syslog" is a roundabout way to ask for
sending the log to _nowhere_, I actually would say that "nonsense"
is a bit too strong a word for the combination of your thing with
"--detach".

It might make more sense to introduce a new "--send-log-to="
option, where the destination can be one of: syslog, stderr, none.

The you can make the current "--syslog" option a synonym to
"--send-log-to=syslog".  The internal variable log_syslog would
probably become

enum log_destination { 
LOG_TO_NONE = -1,
LOG_TO_STDERR = 0,
LOG_TO_SYSLOG = 1,
} log_destination;

and wherever the current code assigns 1 to log_syslog, you would be
setting it LOG_TO_SYSLOG.

Then those who want no log can express that wish in a more direct
way, i.e. "daemon --send-log-to=none", perhaps.

Such an approach leaves open room for future enhancement.  It is not
too far-fetched to imagine something like:

git daemon --send-log-to=/var/log/git-daemon.log

by introducing the fourth value to "enum log_destination"; perhaps
the file is opened and connected to stderr to accept the logs,
combined with a new feature that tells the daemon to close and
reopen the log file when it receives a HUP or something like that.

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Lucas Werkmeister]

On 23.01.2018 23:06, Lucas Werkmeister wrote:
> Ah, I wasn’t aware of that – so with --detach, --no-syslog would be
> better described as “disables all logging” rather than “log to stderr
> instead”. IMHO it would still make sense to have the --no-syslog option
> (then mainly for use with --inetd) as long as its interaction with
> --inetd is properly documented… do you agree? If yes, I’ll be glad to
> submit another version of the patch.

One alternative idea would be to instead make syslog and stderr logging
orthogonal by adding not just --no-syslog, but also --[no-]stderr. For
backwards compatibility, --syslog would imply --no-stderr, but you could
also enable logging to both channels with --syslog --stderr, or disable
all logging with --no-syslog --no-stderr. But that doesn’t really solve
the interaction with --detach – --detach --stderr would still be a
nonsensical combination.



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Lucas Werkmeister]

Thanks for your responses!

On 23.01.2018 01:00, Ævar Arnfjörð Bjarmason wrote:
> This patch looks good, but I wonder if with the rise of systemd
> there's a good reason to flip the default around to not having other
> stuff imply --syslog, and have users specify this implictly, then we
> won't need a --no-syslog option.
> 
> But maybe that'll break too much stuff.
> 

That seems risky to me – even with systemd, the StandardError directive
by default inherits StandardOutput, so if you set StandardOutput=socket
without StandardError=journal, log output in stderr clobbers regular
output. (Also, stderr is apparently closed with --detach, see below.)

On 23.01.2018 19:30, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  writes:
> 
>> On Mon, Jan 22 2018, Lucas Werkmeister jotted:
>> 
>>> Several options imply --syslog, without there being a way to
>>> disable it again. This commit adds that option.
>> 
>> Just two options imply --syslog, --detach & --inetd, unless I've
>> missed something, anyway 2 != several, so maybe just say "The
>> --detach and --inetd options imply --syslog ...".
> 
> Correct.

I respectfully disagree on “2 != several”, but sure, I can repeat the
two options in the message instead :)

> Moreover, --detach completely dissociates the process from the
> original set of standard file descriptors by first closing them and
> then connecting it to "/dev/null", so it will be nonsense to use this
> new option with it.
> 

Ah, I wasn’t aware of that – so with --detach, --no-syslog would be
better described as “disables all logging” rather than “log to stderr
instead”. IMHO it would still make sense to have the --no-syslog option
(then mainly for use with --inetd) as long as its interaction with
--inetd is properly documented… do you agree? If yes, I’ll be glad to
submit another version of the patch.

Best regards,
Lucas



smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Mon, Jan 22 2018, Lucas Werkmeister jotted:
>
>> Several options imply --syslog, without there being a way to disable it
>> again. This commit adds that option.
>
> Just two options imply --syslog, --detach & --inetd, unless I've missed
> something, anyway 2 != several, so maybe just say "The --detach and
> --inetd options imply --syslog ...".

Correct. Moreover, --detach completely dissociates the process from
the original set of standard file descriptors by first closing them
and then connecting it to "/dev/null", so it will be nonsense to use
this new option with it.

Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Lucas Werkmeister jotted:

> Several options imply --syslog, without there being a way to disable it
> again. This commit adds that option.

Just two options imply --syslog, --detach & --inetd, unless I've missed
something, anyway 2 != several, so maybe just say "The --detach and
--inetd options imply --syslog ...".

> This is useful, for instance, when running `git daemon` as a systemd
> service with --inetd. systemd connects stderr to the journal by default,
> so logging to stderr is useful. On the other hand, log messages sent via
> syslog also reach the journal eventually, 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 can no longer read its cgroup and attach the
> message to the correct systemd unit. See systemd/systemd#2913 [1].
>
> [1]: https://github.com/systemd/systemd/issues/2913

This patch looks good, but I wonder if with the rise of systemd there's
a good reason to flip the default around to not having other stuff imply
--syslog, and have users specify this implictly, then we won't need a
--no-syslog option.

But maybe that'll break too much stuff.

[PATCH] daemon: add --no-syslog to undo implicit --syslog

[Lucas Werkmeister]

Several options imply --syslog, without there being a way to disable it
again. This commit adds that option.

This is useful, for instance, when running `git daemon` as a systemd
service with --inetd. systemd connects stderr to the journal by default,
so logging to stderr is useful. On the other hand, log messages sent via
syslog also reach the journal eventually, 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 can no longer read its cgroup and attach the
message to the correct systemd unit. See systemd/systemd#2913 [1].

[1]: https://github.com/systemd/systemd/issues/2913

Signed-off-by: Lucas Werkmeister 
---

Notes:
I decided not to add the option to git-daemon's --help output, since
the similar --no-informative-errors option is also not listed there.
Let me know if it should be added.

Feel free to remove the part about systemd from the commit message
if you feel it doesn't need to be included.

 Documentation/git-daemon.txt | 6 --
 daemon.c | 4 
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..dfd6ce03c 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -8,7 +8,7 @@ git-daemon - A really simple server for Git repositories
 SYNOPSIS
 
 [verse]
-'git daemon' [--verbose] [--syslog] [--export-all]
+'git daemon' [--verbose] [--[no-]syslog] [--export-all]
 [--timeout=] [--init-timeout=] [--max-connections=]
 [--strict-paths] [--base-path=] [--base-path-relaxed]
 [--user-path | --user-path=]
@@ -109,9 +109,11 @@ OPTIONS
Maximum number of concurrent clients, defaults to 32.  Set it to
zero for no limit.
 
---syslog::
+--[no-]syslog::
Log to syslog instead of stderr. Note that this option does not imply
--verbose, thus by default only error conditions will be logged.
+   `--no-syslog` is the default, but may be given explicitly to override
+   the implicit `--syslog` of an earlier `--inetd` or `--detach` option.
 
 --user-path::
 --user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..d59fef6d6 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1300,6 +1300,10 @@ int cmd_main(int argc, const char **argv)
log_syslog = 1;
continue;
}
+   if (!strcmp(arg, "--no-syslog")) {
+   log_syslog = 0;
+   continue;
+   }
if (!strcmp(arg, "--export-all")) {
export_all_trees = 1;
continue;
-- 
2.16.0