From: Eric Sunshine <sunshine@sunshineco.com>
To: Lucas Werkmeister <mail@lucaswerkmeister.de>
Cc: Junio C Hamano <gitster@pobox.com>, Git List <git@vger.kernel.org>
Subject: Re: [PATCH v3] daemon: add --log-destination=(stderr|syslog|none)
Date: Sun, 4 Feb 2018 01:36:17 -0500 [thread overview]
Message-ID: <CAPig+cS8hqaqwrzw9GXY9x0rgksPrCm_deHHw-AL6Tj8-bf1Og@mail.gmail.com> (raw)
In-Reply-To: <20180203230801.30345-1-mail@lucaswerkmeister.de>
On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister
<mail@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 <mail@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=<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 <destination> 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.
> @@ -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);
> @@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)
> + if (skip_prefix(arg, "--log-destination=", &v)) {
> + 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+cTetjQ9LSH68Fe5OTcj9TwQ9GSbGzdrjzHOhTAVFvrPxw@mail.gmail.com/
next prev parent reply other threads:[~2018-02-04 6:38 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-22 23:23 [PATCH] daemon: add --no-syslog to undo implicit --syslog Lucas Werkmeister
2018-01-23 0:00 ` Ævar Arnfjörð Bjarmason
2018-01-23 18:30 ` Junio C Hamano
2018-01-23 22:06 ` Lucas Werkmeister
2018-01-24 10:05 ` Lucas Werkmeister
2018-01-24 18:33 ` Junio C Hamano
2018-01-24 19:48 ` Lucas Werkmeister
2018-01-27 18:31 ` [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none) Lucas Werkmeister
2018-01-28 6:40 ` Eric Sunshine
2018-01-28 22:58 ` Lucas Werkmeister
2018-01-29 0:10 ` Eric Sunshine
2018-02-03 23:08 ` [PATCH v3] daemon: add --log-destination=(stderr|syslog|none) Lucas Werkmeister
2018-02-04 6:36 ` Eric Sunshine [this message]
2018-02-04 18:29 ` Lucas Werkmeister
2018-02-04 18:30 ` [PATCH v4] " Lucas Werkmeister
2018-02-04 18:55 ` Ævar Arnfjörð Bjarmason
2018-02-04 18:58 ` Lucas Werkmeister
2018-02-05 20:09 ` Ævar Arnfjörð Bjarmason
2018-02-04 19:44 ` Eric Sunshine
2018-02-04 19:36 ` Eric Sunshine
2018-02-05 18:31 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPig+cS8hqaqwrzw9GXY9x0rgksPrCm_deHHw-AL6Tj8-bf1Og@mail.gmail.com \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mail@lucaswerkmeister.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).