Skip to content

initialize the log file before reporting config errors#1426

Merged
jedisct1 merged 2 commits into
DNSCrypt:masterfrom
alisonatwork:init-log-before-failing
Jul 27, 2020
Merged

initialize the log file before reporting config errors#1426
jedisct1 merged 2 commits into
DNSCrypt:masterfrom
alisonatwork:init-log-before-failing

Conversation

@alisonatwork

Copy link
Copy Markdown
Member

Further to the discussion in #1413 here is change to the way we load the config file to first initialize the log file before reporting any errors in the config. This will hopefully make it more obvious to people who are running in an automated setup (i.e. Windows or systemd service) why something went wrong after an upgrade to an incompatible version, since normal output to stderr might otherwise get lost.

Here is the output of a manual test to show how it works:

$ ./dnscrypt-proxy.exe -config abc
[2020-07-26 20:48:35] [FATAL] Unable to load the configuration file [abc] -- Maybe use the -config command-line switch?
$ cat > abc <<EOF
log_file = "def"
unknown = "bad_config"
EOF
$ ./dnscrypt-proxy.exe -config abc
$ echo $?
255
$ cat def
[2020-07-26 20:48:48] [NOTICE] dnscrypt-proxy 2.0.44
[2020-07-26 20:48:48] [FATAL] Unsupported key in configuration file: [unknown]

Note that all errors returned during config will result in a fatal log due to code inside main.go (see https://github.com/alisonatwork/dnscrypt-proxy/blob/master/dnscrypt-proxy/main.go#L127) so even very aggressive log levels should still log this kind of failure to the log file or syslog.

To try make the behavior of all config errors resulting in a fatal log more clear to developers, I added a second commit which makes all of the fatal logs just return an error. There is an alternative option, which would be to replace all the "return error" lines with a fatal log. Personally I prefer returning an error, but I don't care too much either way, the main point here is to make all of them consistent.

@jedisct1

Copy link
Copy Markdown
Member

This is a nice improvement!

Thank you Alison!

@jedisct1 jedisct1 merged commit 617629c into DNSCrypt:master Jul 27, 2020
@DNSCrypt DNSCrypt locked and limited conversation to collaborators Aug 26, 2020
@alisonatwork alisonatwork deleted the init-log-before-failing branch April 5, 2021 06:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants