Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)
On Sun, Jan 28, 2018 at 5:58 PM, Lucas Werkmeister
wrote:
> On 28.01.2018 07:40, Eric Sunshine wrote:
>> On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
>> 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
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
> 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?
>
>> Signed-off-by: Lucas Werkmeister
>> Helped-by: Ævar Arnfjörð Bjarmason
>> Helped-by: Junio C Hamano
>> ---
>>
>> 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.
--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= ?
>
> More below...
>
>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
>> @@ -110,8 +111,26 @@ OPTIONS
>> +--send-log-to=::
>> + 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 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=.
Very good idea!
>
>> 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.)
Good point, forgot that part of the coding guidelines. Only the syslog
case needs it because the buf declaration can’t be labeled directly.
>
>> /*
>> * 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 a
Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)
On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
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
> Helped-by: Ævar Arnfjörð Bjarmason
> Helped-by: Junio C Hamano
> ---
>
> 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=::
> + 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 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 cm
[PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)
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.
[1]: https://github.com/systemd/systemd/issues/2913
Signed-off-by: Lucas Werkmeister
Helped-by: Ævar Arnfjörð Bjarmason
Helped-by: Junio C Hamano
---
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?
I’m also not quite sure if the systemd part of the commit message is
accurate – see my comment on the linked issue [2]. TL;DR: this might
no longer be necessary on systemd v235. (I’m experiencing the
problem on Debian Stretch, systemd v232.) As in the last patch, feel
free to remove that part of the commit message.
[2]: https://github.com/systemd/systemd/issues/2913#issuecomment-361002589
Documentation/git-daemon.txt | 23 +--
daemon.c | 43 ---
2 files changed, 57 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..e973f4390 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
[--inetd |
[--listen=] [--port=]
[--user= [--group=]]]
+[--send-log-to=(stderr|syslog|none)]
[...]
DESCRIPTION
@@ -110,8 +111,26 @@ OPTIONS
zero for no limit.
--syslog::
- Log to syslog instead of stderr. Note that this option does not imply
- --verbose, thus by default only error conditions will be logged.
+ Short for `--send-log-to=syslog`.
+
+--send-log-to=::
+ 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 defaults to `stderr`, and must be one of:
++
+--
+stderr::
+ Write to standard error.
+ Note that if `--detach` is specified,
+ the process disconnects from the real standard error,
+ making this destination effectively equivalent to `none`.
+syslog::
+ Write to syslog, using the `git-daemon` identifier.
+none::
+ Disable all logging.
+--
++
--user-path::
--user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..3d8e16ede 100644
--- a/daemon.c
+++ b/daemon.c
@@ -9,7 +9,11 @@
#define initgroups(x, y) (0) /* nothing */
#endif
-static int log_syslog;
+static enum log_destination {
+ LOG_TO_NONE = -1,
+ LOG_TO_STDERR = 0,
+ LOG_TO_SYSLOG = 1,
+} log_destination;
static int verbose;
static int reuseaddr;
static int informative_errors;
@@ -25,6 +29,7 @@ static const char daemon_usage[] =
" [--access-hook=]\n"
" [--inetd | [--listen=] [--port=]\n"
" [--detach] [--user= [--group=]]\n"
+" [--send-log-to=(stderr|syslog|none)]\n"
" [...]";
/* List of acceptable pathname prefixes */
@@ -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: {
/*
* 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;
+ }
}
}
@@ -1289,7 +1302,7 @@ int cmd_main(int argc, const char **argv)
}
