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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 4, 2018, 06:36 UTC
Message-ID: <CAPig+cS8hqaqwrzw9GXY9x0rgksPrCm_deHHw-AL6Tj8-bf1Og@mail.gmail.com>
In-Reply-To: <20180203230801.30345-1-mail@lucaswerkmeister.de>

On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister <mail@lucaswerkmeister.de> wrote:

Show 7 quoted lines

> 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...

Show 7 quoted lines

> 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`, ...

Show 9 quoted lines

> 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.

Show 22 quoted lines

> @@ -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);

Show 13 quoted lines

> @@ -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);

Show 12 quoted lines

> @@ -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/