From 84582bbb887bd8df6436064cd3b69d244c996e15 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 4 Aug 2021 11:11:44 +0200 Subject: [PATCH 1/4] Improve error handling of config file This makes two changes to handling of errors when configuration file could not be loaded: 1. Only NotFound errors are considered OK - access errors and other FS issues are now treated as fatal. 2. Failing to load config file specified explicitly via `--configfile` option is alway a fatal error. Rationale: If the configfile was specified explicitly then it indicates the user really wishes to load it. While the user could want it to be optionally loaded for extra configuration options, this can be accomplished using an empty file. If the config file was not specified explicitly then its' path was computed from loop directory. If the file is inaccessible due to permissions or other FS errors it's nearly certain other following operations will fail as well. Failing early with a clear message is thus beneficial. This still leaves room for uncaught user error (e.g. mistakenly naming config file inside loop dir as `loop.conf` instead of `loopd.conf`) but it's greatly reduced and such error should be easier to identify. (Indirectly) closes #412 --- loopd/run.go | 21 ++++++++++++--------- release_notes.md | 8 ++++++++ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/loopd/run.go b/loopd/run.go index 00bafa3..7e0cee3 100644 --- a/loopd/run.go +++ b/loopd/run.go @@ -149,13 +149,16 @@ func Run(rpcCfg RPCConfig) error { // Parse ini file. loopDir := lncfg.CleanAndExpandPath(config.LoopDir) - configFile := getConfigPath(config, loopDir) + configFile, explicitConfig := getConfigPath(config, loopDir) if err := flags.IniParse(configFile, &config); err != nil { - // If it's a parsing related error, then we'll return - // immediately, otherwise we can proceed as possibly the config - // file doesn't exist which is OK. - if _, ok := err.(*flags.IniError); ok { + // File not existing is OK as long as it wasn't specified + // explicitly. All other errors (parsing, EACCESS...) indicate + // misconfiguration and need to be reported. In case of + // non-not-found FS errors there's high likelihood that other + // operations in data directory would also fail so we treat it + // as early detection of a problem. + if explicitConfig || !os.IsNotExist(err) { return err } } @@ -248,22 +251,22 @@ func Run(rpcCfg RPCConfig) error { // getConfigPath gets our config path based on the values that are set in our // config. -func getConfigPath(cfg Config, loopDir string) string { +func getConfigPath(cfg Config, loopDir string) (string, bool) { // If the config file path provided by the user is set, then we just // use this value. if cfg.ConfigFile != defaultConfigFile { - return lncfg.CleanAndExpandPath(cfg.ConfigFile) + return lncfg.CleanAndExpandPath(cfg.ConfigFile), true } // If the user has set a loop directory that is different to the default // we will use this loop directory as the location of our config file. // We do not namespace by network, because this is a custom loop dir. if loopDir != LoopDirBase { - return filepath.Join(loopDir, defaultConfigFilename) + return filepath.Join(loopDir, defaultConfigFilename), false } // Otherwise, we are using our default loop directory, and the user did // not set a config file path. We use our default loop dir, namespaced // by network. - return filepath.Join(loopDir, cfg.Network, defaultConfigFilename) + return filepath.Join(loopDir, cfg.Network, defaultConfigFilename), false } diff --git a/release_notes.md b/release_notes.md index c71d135..cc82360 100644 --- a/release_notes.md +++ b/release_notes.md @@ -18,6 +18,14 @@ This file tracks release notes for the loop client. #### Breaking Changes +* Failing to load configuration file specified by `--configfile` for any + reason is now hard error. If you've used `--configfile` to mean "optional + extra configuration" you will need to create an empty file. This was done in + [#413](https://github.com/lightninglabs/loop/pull/413) to improve error + reporting and avoid confusion. Similarly, failure to load any configuration + file for reason other than NotFound is hard error, though this is not strictly + breaking because such scenario would be already broken later. + #### Bug Fixes #### Maintenance From 2c1e437f26e85212adae91b17af9a412c4d01818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Habov=C5=A1tiak?= Date: Tue, 10 Aug 2021 08:19:27 +0200 Subject: [PATCH 2/4] Remove extra space Co-authored-by: Yong --- loopd/run.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/loopd/run.go b/loopd/run.go index 7e0cee3..1b9d074 100644 --- a/loopd/run.go +++ b/loopd/run.go @@ -153,7 +153,7 @@ func Run(rpcCfg RPCConfig) error { if err := flags.IniParse(configFile, &config); err != nil { // File not existing is OK as long as it wasn't specified - // explicitly. All other errors (parsing, EACCESS...) indicate + // explicitly. All other errors (parsing, EACCESS...) indicate // misconfiguration and need to be reported. In case of // non-not-found FS errors there's high likelihood that other // operations in data directory would also fail so we treat it From c00da1cf092fb08bd6f32b05b4e007085f63a920 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 10 Aug 2021 08:23:36 +0200 Subject: [PATCH 3/4] Renamed explicitConfig to hasExplicitConfig `hasExplicitConfig` is considered cleaner. --- loopd/run.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/loopd/run.go b/loopd/run.go index 1b9d074..f17cfae 100644 --- a/loopd/run.go +++ b/loopd/run.go @@ -149,7 +149,7 @@ func Run(rpcCfg RPCConfig) error { // Parse ini file. loopDir := lncfg.CleanAndExpandPath(config.LoopDir) - configFile, explicitConfig := getConfigPath(config, loopDir) + configFile, hasExplicitConfig := getConfigPath(config, loopDir) if err := flags.IniParse(configFile, &config); err != nil { // File not existing is OK as long as it wasn't specified @@ -158,7 +158,7 @@ func Run(rpcCfg RPCConfig) error { // non-not-found FS errors there's high likelihood that other // operations in data directory would also fail so we treat it // as early detection of a problem. - if explicitConfig || !os.IsNotExist(err) { + if hasExplicitConfig || !os.IsNotExist(err) { return err } } From 158e22f2a7de41f03559e681832f30a63f9b6fc6 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 10 Aug 2021 08:24:31 +0200 Subject: [PATCH 4/4] Documented the menaing of returned bool --- loopd/run.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/loopd/run.go b/loopd/run.go index f17cfae..ff445fb 100644 --- a/loopd/run.go +++ b/loopd/run.go @@ -250,7 +250,8 @@ func Run(rpcCfg RPCConfig) error { } // getConfigPath gets our config path based on the values that are set in our -// config. +// config. The returned bool is set to true if the config file path was set +// explicitly by the user and thus should not be ignored if it doesn't exist. func getConfigPath(cfg Config, loopDir string) (string, bool) { // If the config file path provided by the user is set, then we just // use this value.