Re: [PATCH] Fix condition for redirecting stderr

[Lucas Werkmeister]

On 09.04.2018 04:23, Todd Zullinger wrote:
> Lucas Werkmeister wrote:
>> Since the --log-destination option was added in 0c591cacb ("daemon: add
>> --log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit
>> goal of allowing logging to stderr when running in inetd mode, we should
>> not always redirect stderr to /dev/null in inetd mode, but rather only
>> when stderr is not being used for logging.
> 
> Perhaps 's/^F/daemon: f/' on the subject?  (Junio may well
> already have done so while queueing locally.)

Indeed, sorry! Looks like Junio fixed it on his end (going by the
“what’s cooking” email), so I won’t reroll. (But I can at least set up a
local commit-msg hook to make sure I don’t forget the subject area again.)

[PATCH] Fix condition for redirecting stderr

[Lucas Werkmeister]

Since the --log-destination option was added in 0c591cacb ("daemon: add
--log-destination=(stderr|syslog|none)", 2018-02-04) with the explicit
goal of allowing logging to stderr when running in inetd mode, we should
not always redirect stderr to /dev/null in inetd mode, but rather only
when stderr is not being used for logging.

Signed-off-by: Lucas Werkmeister <m...@lucaswerkmeister.de>
---

Notes:
I have to apologize to the list here… even though I proposed this
option with the goal of using it on my server, for some reason I
decided to only use it there after the next Git release had come
out, and didn’t test it anywhere else. The code looked okay, but I
missed this part near the end of daemon.c that made it ineffective,
rendering the feature broken in the 2.17.0 release and making me
look like an idiot :/ sorry, everyone.

For what it’s worth, with this fix the feature appears to work as
intended (I *have* tested it on my server now and log messages from
git-daemon show up in the journal as intended, attributed to the
correct unit and everything).

 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index fe833ea7d..9d2e0d20e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1459,7 +1459,7 @@ int cmd_main(int argc, const char **argv)
die("base-path '%s' does not exist or is not a directory",
base_path);
 
-   if (inetd_mode) {
+   if (log_destination != LOG_DESTINATION_STDERR) {
if (!freopen("/dev/null", "w", stderr))
die_errno("failed to redirect stderr to /dev/null");
}
-- 
2.16.3

Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

[Lucas Werkmeister]

On 04.02.2018 19:55, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Feb 04 2018, Lucas Werkmeister jotted:
> 
>>   [--inetd |
>>[--listen=] [--port=]
>>[--user= [--group=]]]
>> + [--log-destination=(stderr|syslog|none)]
> 
> I micronit, but maybe worthwhile to have a preceeding commit to fix up
> that indentation of --listen and --user.

I thought the indentation here is intentional, since we’re still inside
the [] pair (either --inetd or --listen, --port, …).

> 
>> +--log-destination=::
>> +Send log messages to the specified destination.
>> +Note that this option does not imply --verbose,
> 
> Should `` quote --verbose, although I see similar to the WS change I
> noted above there's plenty of existing stuff in that doc doing it wrong.

I could send a follow-up to consistently ``-quote all options in
git-daemon.txt… or would that be rejected as unnecessarily cluttering
the history or the `git blame`?

>> +} else
>> +die("unknown log destination '%s'", v);
> 
> Should be die(_("unknown..., i.e. use the _() macro.
> 
> Anyway, this looks fine to be with our without these proposed
> bikeshedding changes above. Thanks.
>



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

[Lucas Werkmeister]

This new option can be used to override the implicit --syslog of
--inetd, or to disable all logging. (While --detach also implies
--syslog, --log-destination=stderr with --detach is useless since
--detach disassociates the process from the original stderr.) --syslog
is retained as an alias for --log-destination=syslog.

--log-destination always overrides implicit --syslog regardless of
option order. This is different than the “last one wins” logic that
applies to some implicit options elsewhere in Git, but should hopefully
be less confusing. (I also don’t know if *all* implicit options in Git
follow “last one wins”.)

The combination of --inetd with --log-destination=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

Helped-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
Helped-by: Junio C Hamano <gits...@pobox.com>
Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Lucas Werkmeister <m...@lucaswerkmeister.de>
---

Notes:
Fixes log_destination not being initialized correctly and some minor
style issues.

 Documentation/git-daemon.txt | 28 ---
 daemon.c | 46 +---
 2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..56d54a489 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 [--inetd |
  [--listen=] [--port=]
  [--user= [--group=]]]
+[--log-destination=(stderr|syslog|none)]
 [...]
 
 DESCRIPTION
@@ -80,7 +81,8 @@ OPTIONS
do not have the 'git-daemon-export-ok' file.
 
 --inetd::
-   Have the server run as an inetd service. Implies --syslog.
+   Have the server run as an inetd service. Implies --syslog (may be
+   overridden with `--log-destination=`).
Incompatible with --detach, --port, --listen, --user and --group
options.
 
@@ -110,8 +112,28 @@ 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 `--log-destination=syslog`.
+
+--log-destination=::
+   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  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.
+--
++
+The default destination is `syslog` if `--inetd` or `--detach` is specified,
+otherwise `stderr`.
 
 --user-path::
 --user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..fb538e367 100644
--- a/daemon.c
+++ b/daemon.c
@@ -9,7 +9,12 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
-static int log_syslog;
+static enum log_destination {
+   LOG_DESTINATION_UNSET = -1,
+   LOG_DESTINATION_NONE = 0,
+   LOG_DESTINATION_STDERR = 1,
+   LOG_DESTINATION_SYSLOG = 2,
+} log_destination = LOG_DESTINATION_UNSET;
 static int verbose;
 static int reuseaddr;
 static int informative_errors;
@@ -25,6 +30,7 @@ static const char daemon_usage[] =
 "   [--access-hook=]\n"
 "   [--inetd | [--listen=] [--port=]\n"
 "  [--detach] [--user= [--group=]]\n"
+"   [--log-destination=(stderr|syslog|none)]\n"
 "   [...]";
 
 /* List of acceptable pathname prefixes */
@@ -74,11 +80,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_DESTINATION_SYSLOG: {
char buf[1024];
vsnprintf(buf, sizeof(buf), err, params);
syslog(priority, "%s", buf);
-   } else {
+   break;
+   }
+   ca

Re: [PATCH v3] daemon: add --log-destination=(stderr|syslog|none)

[Lucas Werkmeister]

On 04.02.2018 07:36, Eric Sunshine wrote:
> On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister
> <m...@lucaswerkmeister.de> wrote:
>> This new option can be used to override the implicit --syslog of
>> --inetd, or to disable all logging. (While --detach also implies
>> --syslog, --log-destination=stderr with --detach is useless since
>> --detach disassociates the process from the original stderr.) --syslog
>> is retained as an alias for --log-destination=syslog.
>> [...]
>> Signed-off-by: Lucas Werkmeister <m...@lucaswerkmeister.de>
> 
> Thanks for the re-roll. There are a few comments below. Except for one
> apparent bug, I'm not sure the others are worth a re-roll...
> 
>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
>> @@ -110,8 +112,26 @@ OPTIONS
>> +--log-destination=::
>> +   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:
> 
> I wonder if this should say instead:
> 
> The default destination is `stderr` unless `syslog`
> is implied by `--inetd` or `--detach`, ...
> 
>> diff --git a/daemon.c b/daemon.c
>> @@ -9,7 +9,12 @@
>> -static int log_syslog;
>> +static enum log_destination {
>> +   LOG_DESTINATION_UNSET = -1,
>> +   LOG_DESTINATION_NONE = 0,
>> +   LOG_DESTINATION_STDERR = 1,
>> +   LOG_DESTINATION_SYSLOG = 2,
>> +} log_destination;
> 
> Doesn't log_destination need to be initialized to
> LOG_DESTINATION_UNSET (see [1])? As it stands, being static, it's
> initialized automatically to 0 (LOG_DESTINATION_NONE), which borks the
> logic below.

Thanks, I knew I’d forgotten something :)

> 
>> @@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)
>>  static void logreport(int priority, const char *err, va_list params)
>>  {
>> +   switch (log_destination) {
>> +   case LOG_DESTINATION_SYSLOG: {
>> char buf[1024];
>> vsnprintf(buf, sizeof(buf), err, params);
>> syslog(priority, "%s", buf);
>> +   break;
>> +   }
>> +   case LOG_DESTINATION_STDERR:
>> /*
>>  * Since stderr is set to buffered mode, the
>>  * logging of different processes will not overlap
>> @@ -88,6 +97,10 @@ static void logreport(int priority, const char *err, 
>> va_list params)
>> vfprintf(stderr, err, params);
>> fputc('\n', stderr);
>> fflush(stderr);
>> +   break;
>> +   case LOG_DESTINATION_NONE:
>> +   case LOG_DESTINATION_UNSET:
>> +   break;
> 
> Since LOG_DESTINATION_UNSET should never occur, perhaps this should be
> written as:
> 
> case LOG_DESTINATION_NONE:
> break;
> case LOG_DESTINATION_UNSET:
> BUG("impossible destination: %d", log_destination);

Good point, I didn’t know about the BUG() macro. But putting the
destination in the message seems unnecessary if it can only be a single
value – or would you make this a default: case?

> 
>> @@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)
>> +   if (skip_prefix(arg, "--log-destination=", )) {
>> +   if (!strcmp(v, "syslog")) {
>> +   log_destination = LOG_DESTINATION_SYSLOG;
>> +   continue;
>> +   } else if (!strcmp(v, "stderr")) {
>> +   log_destination = LOG_DESTINATION_STDERR;
>> +   continue;
>> +   } else if (!strcmp(v, "none")) {
>> +   log_destination = LOG_DESTINATION_NONE;
>> +   continue;
>> +   } else
>> +   die("Unknown log destination %s", v);
> 
> Mentioned previously[1], this probably ought to start with lowercase.
> It also would be more readable to set off the unknown value with a
> colon or quotes:
> 
> die("unknown log destination '%s', v);
> 
>> @@ -1402,7 +1426,14 @@ int cmd_main(int argc, const char **argv)
>> -   if (log_syslog) {
>> +   if (log_destination == LOG_DESTINATION_UNSET) {
>> +   if (inetd_mode || detach)
>> +   log_destination = LOG_DESTINATION_SYSLOG;
>> +   else
>> +   log_destination = LOG_DESTINATION_STDERR;
>> +   }
>> +
>> +   if (log_destination == LOG_DESTINATION_SYSLOG) {
>> openlog("git-daemon", LOG_PID, LOG_DAEMON);
>> set_die_routine(daemon_die);
> 
> [1]: 
> https://public-inbox.org/git/capig+ctetjq9lsh68fe5otcj9twq9gsbgzdrjzhohtavfvr...@mail.gmail.com/
> 

I’ll send a new version shortly, also addressing your other comments
which I didn’t reply to here.

Cheers,
Lucas



smime.p7s
Description: S/MIME Cryptographic Signature

Re: contrib/completion/git-completion.bash: declare -g is not portable

[Lucas Werkmeister]

On 04.02.2018 10:57, Eric Sunshine wrote:
> On Sun, Feb 4, 2018 at 4:45 AM, Duy Nguyen  wrote:
>> On Sun, Feb 4, 2018 at 12:20 AM, Torsten Bögershausen  wrote:
>>> After running t9902-completion.sh on Mac OS I got a failure
>>> in this style:
>>
>> Sorry I was new with this bash thingy. Jeff already answered this (and
>> I will fix it in the re-roll) but just for my own information, what
>> bash version is shipped with Mac OS?
> 
> The MacOS bash is very old; note the copyright date:
> 
> % /bin/bash --version
> GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin16)
> Copyright (C) 2007 Free Software Foundation, Inc.
> 
> A recent bash installed manually (not from Apple):
> 
> % /usr/local/bin/bash --version
> GNU bash, version 4.4.18(1)-release (x86_64-apple-darwin16.7.0)
> Copyright (C) 2016 Free Software Foundation, Inc.
> 

Specifically, Apple ships the latest version of Bash 3.x, which is the
last version published under the GPLv2+ – Bash 4.x switched to GPLv3+.
Users can install newer Bash versions themselves, e. g. using Homebrew,
but that doesn’t help us here.



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v3] daemon: add --log-destination=(stderr|syslog|none)

[Lucas Werkmeister]

This new option can be used to override the implicit --syslog of
--inetd, or to disable all logging. (While --detach also implies
--syslog, --log-destination=stderr with --detach is useless since
--detach disassociates the process from the original stderr.) --syslog
is retained as an alias for --log-destination=syslog.

--log-destination always overrides implicit --syslog regardless of
option order. This is different than the “last one wins” logic that
applies to some implicit options elsewhere in Git, but should hopefully
be less confusing. (I also don’t know if *all* implicit options in Git
follow “last one wins”.)

The combination of --inetd with --log-destination=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

Helped-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
Helped-by: Junio C Hamano <gits...@pobox.com>
Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Lucas Werkmeister <m...@lucaswerkmeister.de>
---

Notes:
Next version on my quest to make git-daemon logging more reliable :)
many thanks to Eric Sunshine for review. Main changes this version:

- Rename --send-log-to to --log-destination, following the
  postgresql option. A cursory search didn’t turn up any other
  daemons with a similar option – suggestions welcome!
- Make explicit --log-destination always override implicit --syslog,
  regardless of option order.

 Documentation/git-daemon.txt | 26 ++---
 daemon.c | 45 +---
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..a0106e6aa 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 [--inetd |
  [--listen=] [--port=]
  [--user= [--group=]]]
+[--log-destination=(stderr|syslog|none)]
 [...]
 
 DESCRIPTION
@@ -80,7 +81,8 @@ OPTIONS
do not have the 'git-daemon-export-ok' file.
 
 --inetd::
-   Have the server run as an inetd service. Implies --syslog.
+   Have the server run as an inetd service. Implies --syslog (may be
+   overridden with `--log-destination=`).
Incompatible with --detach, --port, --listen, --user and --group
options.
 
@@ -110,8 +112,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 `--log-destination=syslog`.
+
+--log-destination=::
+   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..f28400e3f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -9,7 +9,12 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
-static int log_syslog;
+static enum log_destination {
+   LOG_DESTINATION_UNSET = -1,
+   LOG_DESTINATION_NONE = 0,
+   LOG_DESTINATION_STDERR = 1,
+   LOG_DESTINATION_SYSLOG = 2,
+} log_destination;
 static int verbose;
 static int reuseaddr;
 static int informative_errors;
@@ -25,6 +30,7 @@ static const char daemon_usage[] =
 "   [--access-hook=]\n"
 "   [--inetd | [--listen=] [--port=]\n"
 "  [--detach] [--user= [--group=]]\n"
+"   [--log-destination=(stderr|syslog|none)]\n"
 "   [...]";
 
 /* List of acceptable pathname prefixes */
@@ -74,11 +80,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_dest

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

[Lucas Werkmeister]

On 28.01.2018 07:40, Eric Sunshine wrote:
> On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
> <m...@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?

> 
>> Signed-off-by: Lucas Werkmeister <m...@lucaswerkmeister.de>
>> Helped-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
>> Helped-by: Junio C Hamano <gits...@pobox.com>
>> ---
>>
>> 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 rathe

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

[Lucas Werkmeister]

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 <m...@lucaswerkmeister.de>
Helped-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
Helped-by: Junio C Hamano <gits...@pobox.com>
---

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: {
+   

Re: [PATCH 2/6] t/lib-git-daemon: record daemon log

[Lucas Werkmeister]

On 25.01.2018 01:55, Jeff King wrote:
> When we start git-daemon for our tests, we send its stderr
> log stream to a named pipe. We synchronously read the first
> line to make sure that the daemon started, and then dump the
> rest to descriptor 4. This is handy for debugging test
> output with "--verbose", but the tests themselves can't
> access the log data.
> 
> Let's dump the log into a file, as well, so that future
> tests can check the log. There are two subtleties worth
> calling out here:
> 
>   - we replace "cat" with a subshell loop around "read" to
> ensure that there's no buffering (so that tests can be
> sure that after a request has been served, the matching
> log entries will have made it to the file)

POSIX specifies the -u option for that behavior, can’t you use that?
(GNU coreutils’ cat ignores it, since writing without delay is
apparently its default behavior already.)

> 
>   - we open the logfile for append, rather than just output.
> That makes it OK for tests to truncate the logfile
> without restarting the daemon (the OS will atomically
> seek to the end of the file when outputting each line).
> That allows tests to look at the log without worrying
> about pollution from earlier tests.
> 
> Signed-off-by: Jeff King 
> ---
>  t/lib-git-daemon.sh | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
> index 987d40680b..19f3ffdbb1 100644
> --- a/t/lib-git-daemon.sh
> +++ b/t/lib-git-daemon.sh
> @@ -53,11 +53,19 @@ start_git_daemon() {
>   "$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
>   >&3 2>git_daemon_output &
>   GIT_DAEMON_PID=$!
> + >daemon.log
>   {
>   read line <&7
> + echo "$line"
>   echo >&4 "$line"
> - cat <&7 >&4 &
> - } 7 + (
> + while read line <&7
> + do
> + echo "$line"
> + echo >&4 "$line"
> + done
> + ) &
> + } 7>"$TRASH_DIRECTORY/daemon.log" &&
>  
>   # Check expected output
>   if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
> 

read without -r clobbers backslashes, and echo may interpret escape
sequences. To faithfully reproduce the output, it would be better to use
read -r and printf '%s\n' "$line", I think. (However, it looks like the
existing code already uses read+echo, so I guess you could also keep
that pattern in this change and then fix it in a later one.)

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

[Lucas Werkmeister]

On 24.01.2018 19:33, Junio C Hamano wrote:
> Lucas Werkmeister <m...@lucaswerkmeister.de> 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

[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 <ava...@gmail.com> 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

[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 <m...@lucaswerkmeister.de>
---

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