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

[Lucas Werkmeister <mail@lucaswerkmeister.de>]

[-- Attachment #1: Type: text/plain, Size: 5087 bytes --]

On 04.02.2018 07:36, Eric Sunshine wrote:
> 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.

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=", &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/
> 

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

Cheers,
Lucas


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4165 bytes --]