New -includeconf argument for including external configuration files #10267

Open
wants to merge 3 commits into
from
View
@@ -44,6 +44,10 @@ Specify configuration file (default: bitcoin.conf)
.IP
Specify data directory
.HP
+\fB\-includeconf=\fR<dir>
+.IP
+Specify additional configuration file, relative to the \fB\-datadir\fR path
+.HP
\fB\-dbcache=\fR<n>
.IP
Set database cache size in megabytes (4 to 16384, default: 300)
View
@@ -49,6 +49,10 @@ Run in the background as a daemon and accept commands
.IP
Specify data directory
.HP
+\fB\-includeconf=\fR<dir>
+.IP
+Specify additional configuration file, relative to the \fB\-datadir\fR path
+.HP
\fB\-dbcache=\fR<n>
.IP
Set database cache size in megabytes (4 to 16384, default: 300)
View
@@ -103,7 +103,7 @@ static int AppInitRPC(int argc, char* argv[])
return EXIT_FAILURE;
}
try {
- ReadConfigFile(GetArg("-conf", BITCOIN_CONF_FILENAME));
+ ReadConfigFiles();
} catch (const std::exception& e) {
fprintf(stderr,"Error reading configuration file: %s\n", e.what());
return EXIT_FAILURE;
View
@@ -103,7 +103,7 @@ bool AppInit(int argc, char* argv[])
}
try
{
- ReadConfigFile(GetArg("-conf", BITCOIN_CONF_FILENAME));
+ ReadConfigFiles();
} catch (const std::exception& e) {
fprintf(stderr,"Error reading configuration file: %s\n", e.what());
return false;
View
@@ -353,6 +353,7 @@ std::string HelpMessage(HelpMessageMode mode)
if (showDebug) {
strUsage += HelpMessageOpt("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize));
}
+ strUsage += HelpMessageOpt("-includeconf=<file>", _("Specify additional configuration file, relative to the -datadir path"));
strUsage += HelpMessageOpt("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache));
if (showDebug)
strUsage += HelpMessageOpt("-feefilter", strprintf("Tell other nodes to filter invs to us by our mempool min fee (default: %u)", DEFAULT_FEEFILTER));
View
@@ -617,7 +617,7 @@ int main(int argc, char *argv[])
return EXIT_FAILURE;
}
try {
- ReadConfigFile(GetArg("-conf", BITCOIN_CONF_FILENAME));
+ ReadConfigFiles();
} catch (const std::exception& e) {
QMessageBox::critical(0, QObject::tr(PACKAGE_NAME),
QObject::tr("Error: Cannot parse configuration file: %1. Only use key=value syntax.").arg(e.what()));
@@ -359,6 +359,7 @@ QT_TRANSLATE_NOOP("bitcoin-core", "Signing transaction failed"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specify configuration file (default: %s)"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specify connection timeout in milliseconds (minimum: 1, default: %d)"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specify data directory"),
+QT_TRANSLATE_NOOP("bitcoin-core", "Specify additional configuration file, relative to the -datadir path"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specify pid file (default: %s)"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specify wallet file (within data directory)"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specify your own public address"),
View
@@ -598,26 +598,47 @@ fs::path GetConfigFile(const std::string& confPath)
return pathConfigFile;
}
-void ArgsManager::ReadConfigFile(const std::string& confPath)
+void ArgsManager::ReadConfigFile(fs::ifstream& streamConfig)
+{
+ std::set<std::string> setOptions;
+ setOptions.insert("*");
+
+ for (boost::program_options::detail::config_file_iterator it(streamConfig, setOptions), end; it != end; ++it) {
+ // Don't overwrite existing settings so command line settings override bitcoin.conf
+ std::string strKey = std::string("-") + it->string_key;
+ std::string strValue = it->value[0];
+ InterpretNegativeSetting(strKey, strValue);
+ if (mapArgs.count(strKey) == 0) {
+ mapArgs[strKey] = strValue;
+ }
+ mapMultiArgs[strKey].push_back(strValue);
+ }
+}
+
+void ArgsManager::ReadConfigFiles()
{
+ const std::string confPath = GetArg("-conf", BITCOIN_CONF_FILENAME);
fs::ifstream streamConfig(GetConfigFile(confPath));
if (!streamConfig.good())
return; // No bitcoin.conf file is OK
+ // we do not allow -includeconf from command line, so we clear it here
+ mapArgs.erase("-includeconf");
+
+ std::string includeconf;
{
LOCK(cs_args);
- std::set<std::string> setOptions;
- setOptions.insert("*");
-
- for (boost::program_options::detail::config_file_iterator it(streamConfig, setOptions), end; it != end; ++it)
- {
- // Don't overwrite existing settings so command line settings override bitcoin.conf
- std::string strKey = std::string("-") + it->string_key;
- std::string strValue = it->value[0];
- InterpretNegativeSetting(strKey, strValue);
- if (mapArgs.count(strKey) == 0)
- mapArgs[strKey] = strValue;
- mapMultiArgs[strKey].push_back(strValue);
+ ReadConfigFile(streamConfig);
+ if (mapArgs.count("-includeconf")) includeconf = mapArgs["-includeconf"];
+ }
+ if (includeconf != "") {
+ LogPrintf("Attempting to include configuration file %s\n", includeconf.c_str());
+ fs::ifstream includeConfig(GetConfigFile(includeconf));
+ if (includeConfig.good()) {
+ LOCK(cs_args);
+ ReadConfigFile(includeConfig);
+ } else {
+ LogPrintf("Failed to include configuration file %s\n", includeconf.c_str());
}
}
// If datadir is changed in .conf file:
@jtimon

jtimon Jun 1, 2017 edited

Member

Shouldn't ClearDatadirCache() be called from ArgsManager::ReadConfigFile(fs::ifstream& streamConfig) ?

@kallewoof

kallewoof Jun 5, 2017

Contributor

ReadConfigFile is only called from ReadConfigFiles, which clears the data cache already, so it should be fine I think.

@jtimon

jtimon Jun 5, 2017

Member

Yes, unless you conserve a ArgsManager::ReadConfigFile(const std::string& confPath), then that will presumably call call ArgsManager::ReadConfigFile(fs::ifstream& streamConfig too.

View
@@ -194,13 +194,15 @@ inline bool IsSwitchChar(char c)
class ArgsManager
{
+private:
+ void ReadConfigFile(fs::ifstream& streamConfig);
protected:
CCriticalSection cs_args;
std::map<std::string, std::string> mapArgs;
std::map<std::string, std::vector<std::string> > mapMultiArgs;
public:
void ParseParameters(int argc, const char*const argv[]);
- void ReadConfigFile(const std::string& confPath);
+ void ReadConfigFiles();
std::vector<std::string> GetArgs(const std::string& strArg);
/**
@@ -269,9 +271,9 @@ static inline void ParseParameters(int argc, const char*const argv[])
gArgs.ParseParameters(argc, argv);
}
-static inline void ReadConfigFile(const std::string& confPath)
+static inline void ReadConfigFiles()
{
- gArgs.ReadConfigFile(confPath);
+ gArgs.ReadConfigFiles();
}
static inline bool SoftSetArg(const std::string& strArg, const std::string& strValue)
@@ -0,0 +1,39 @@
+#!/usr/bin/env python3
@jnewbery

jnewbery May 31, 2017

Member

It'd be nice to have a couple of other subtests:

  • including --includeconf in the command line argument has no effect
  • including includeconf in an includeconf file has no effect.
@kallewoof

kallewoof Jun 1, 2017 edited

Contributor

I'm not sure what a neat and tidy way to test that would look like. Should I just make multiple classes, one for each, and then call them one at a time in the bottom if __name__ == '__main__':?

@jnewbery

jnewbery Jun 1, 2017

Member

These don't actually need to be separate tests. You can just have the following in setup_chain():

        with open(os.path.join(self.options.tmpdir + "/node0", "relative.conf"), "w", encoding="utf8") as f:
            f.write("uacomment=relative\nincludeconf=relativeofrelative.conf\n")
        with open(os.path.join(self.options.tmpdir + "/node0", "bitcoin.conf"), 'a', encoding='utf8') as f:
            f.write("uacomment=main\nincludeconf=relative.conf\n")
        with open(os.path.join(self.options.tmpdir + "/node0", "commandline.conf"), "w", encoding="utf8") as f:
            f.write("uacomment=commandline\n")
        with open(os.path.join(self.options.tmpdir + "/node0", "relativeofrelative.conf"), "w", encoding="utf8") as f:
            f.write("uacomment=relativeofrelative\n")

and the following line in __init__():

        self.extra_args = [['-includeconf=commandline.conf']]

and then keep the same assert:

        assert subversion.endswith("main; relative)/")

to check that neither comandline.conf nor relativeofrelative.conf have been included.

(I recommend you also update the docstring to describe what's happening)

+# Copyright (c) 2017 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Tests the includeconf directive
+
+Create an additional configuration file that is loaded from the main
+bitcoin.conf via includeconf (done in setup_chain). We check that:
+1. The files (bitcoin.conf and relative.conf) are indeed loaded.
+2. The load ordering is correct (bitcoin.conf, then relative).
+"""
+
+import os
+from test_framework.test_framework import BitcoinTestFramework
+
+class IncludeConfTest(BitcoinTestFramework):
+ def setup_chain(self):
+ super().setup_chain()
+ # Create additional config file
+ # - tmpdir/node0/relative.conf
+ with open(os.path.join(self.options.tmpdir + "/node0", "relative.conf"), "w", encoding="utf8") as f:
+ f.write("uacomment=relative\n")
+ with open(os.path.join(self.options.tmpdir + "/node0", "bitcoin.conf"), 'a', encoding='utf8') as f:
+ f.write("uacomment=main\nincludeconf=relative.conf\n")
+ # subversion should end with "(main; relative)/"
+
+ def __init__(self):
+ super().__init__()
+ self.setup_clean_chain = False
+ self.num_nodes = 1
+
+ def run_test(self):
+ nwinfo = self.nodes[0].getnetworkinfo()
+ subversion = nwinfo["subversion"]
+ assert subversion.endswith("main; relative)/")
+
+if __name__ == '__main__':
+ IncludeConfTest().main()
@@ -115,6 +115,7 @@
'p2p-leaktests.py',
'wallet-encryption.py',
'uptime.py',
+ 'includeconf.py',
]
EXTENDED_SCRIPTS = [