On 04.02.2018 07:36, Eric Sunshine wrote: > On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister > <[email protected]> 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 <[email protected]> > > 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+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
