-
Notifications
You must be signed in to change notification settings - Fork 37.7k
New -includeconf argument for including external configuration files #10267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
Changed command-line options | ||
---------------------------- | ||
|
||
- `-includeconf=<file>` can be used to include additional configuration files. | ||
Only works inside the `bitcoin.conf` file, not inside included files or from | ||
command-line. Multiple files may be included. Can be disabled from command- | ||
line via `-noincludeconf`. Note that multi-argument commands like | ||
`-includeconf` will override preceding `-noincludeconf`, i.e. | ||
|
||
noincludeconf=1 | ||
includeconf=relative.conf | ||
|
||
as bitcoin.conf will still include `relative.conf`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -445,6 +445,17 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[]) | |
m_override_args[key].push_back(val); | ||
} | ||
} | ||
|
||
// we do not allow -includeconf from command line, so we clear it here | ||
auto it = m_override_args.find("-includeconf"); | ||
if (it != m_override_args.end()) { | ||
if (it->second.size() > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be nice if we could be more aggressive here and fail to start if a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I'm leaving it for now as it sounds like a future thing, I think. |
||
for (const auto& ic : it->second) { | ||
fprintf(stderr, "warning: -includeconf cannot be used from commandline; ignoring -includeconf=%s\n", ic.c_str()); | ||
} | ||
m_override_args.erase(it); | ||
} | ||
} | ||
} | ||
|
||
std::vector<std::string> ArgsManager::GetArgs(const std::string& strArg) const | ||
|
@@ -706,18 +717,40 @@ void ArgsManager::ReadConfigStream(std::istream& stream) | |
} | ||
} | ||
|
||
void ArgsManager::ReadConfigFile(const std::string& confPath) | ||
void ArgsManager::ReadConfigFiles() | ||
{ | ||
{ | ||
LOCK(cs_args); | ||
m_config_args.clear(); | ||
} | ||
|
||
const std::string confPath = GetArg("-conf", BITCOIN_CONF_FILENAME); | ||
fs::ifstream stream(GetConfigFile(confPath)); | ||
|
||
// ok to not have a config file | ||
if (stream.good()) { | ||
ReadConfigStream(stream); | ||
// if there is an -includeconf in the override args, but it is empty, that means the user | ||
// passed '-noincludeconf' on the command line, in which case we should not include anything | ||
if (m_override_args.count("-includeconf") == 0) { | ||
std::vector<std::string> includeconf(GetArgs("-includeconf")); | ||
{ | ||
// We haven't set m_network yet (that happens in SelectParams()), so manually check | ||
// for network.includeconf args. | ||
std::vector<std::string> includeconf_net(GetArgs(std::string("-") + GetChainName() + ".includeconf")); | ||
includeconf.insert(includeconf.end(), includeconf_net.begin(), includeconf_net.end()); | ||
} | ||
|
||
for (const std::string& to_include : includeconf) { | ||
fs::ifstream include_config(GetConfigFile(to_include)); | ||
if (include_config.good()) { | ||
ReadConfigStream(include_config); | ||
LogPrintf("Included configuration file %s\n", to_include.c_str()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} else { | ||
fprintf(stderr, "Failed to include configuration file %s\n", to_include.c_str()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: return false on error? |
||
} | ||
} | ||
} | ||
} | ||
|
||
// If datadir is changed in .conf file: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
#!/usr/bin/env python3 | ||
# Copyright (c) 2018 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 argument | ||
|
||
Verify that: | ||
|
||
1. adding includeconf to the configuration file causes the includeconf | ||
file to be loaded in the correct order. | ||
2. includeconf cannot be used as a command line argument. | ||
3. includeconf cannot be used recursively (ie includeconf can only | ||
be used from the base config file). | ||
4. multiple includeconf arguments can be specified in the main config | ||
file. | ||
""" | ||
import os | ||
import tempfile | ||
|
||
from test_framework.test_framework import BitcoinTestFramework, assert_equal | ||
|
||
class IncludeConfTest(BitcoinTestFramework): | ||
def set_test_params(self): | ||
self.setup_clean_chain = False | ||
self.num_nodes = 1 | ||
|
||
def setup_chain(self): | ||
super().setup_chain() | ||
# Create additional config files | ||
# - 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") | ||
# - tmpdir/node0/relative2.conf | ||
with open(os.path.join(self.options.tmpdir, "node0", "relative2.conf"), "w", encoding="utf8") as f: | ||
f.write("uacomment=relative2\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") | ||
|
||
def run_test(self): | ||
self.log.info("-includeconf works from config file. subversion should end with 'main; relative)/'") | ||
|
||
subversion = self.nodes[0].getnetworkinfo()["subversion"] | ||
assert subversion.endswith("main; relative)/") | ||
|
||
self.log.info("-includeconf cannot be used as command-line arg. subversion should still end with 'main; relative)/'") | ||
self.stop_node(0) | ||
with tempfile.SpooledTemporaryFile(max_size=2**16) as log_stderr: | ||
self.start_node(0, extra_args=["-includeconf=relative2.conf"], stderr=log_stderr) | ||
|
||
subversion = self.nodes[0].getnetworkinfo()["subversion"] | ||
assert subversion.endswith("main; relative)/") | ||
log_stderr.seek(0) | ||
stderr = log_stderr.read().decode('utf-8').strip() | ||
assert_equal(stderr, 'warning: -includeconf cannot be used from commandline; ignoring -includeconf=relative2.conf') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should probably be an (init)error, since it is clearly an invalid command line |
||
|
||
self.log.info("-includeconf cannot be used recursively. subversion should end with 'main; relative)/'") | ||
with open(os.path.join(self.options.tmpdir, "node0", "relative.conf"), "a", encoding="utf8") as f: | ||
f.write("includeconf=relative2.conf\n") | ||
|
||
self.restart_node(0) | ||
|
||
subversion = self.nodes[0].getnetworkinfo()["subversion"] | ||
assert subversion.endswith("main; relative)/") | ||
|
||
self.log.info("multiple -includeconf args can be used from the base config file. subversion should end with 'main; relative; relative2)/'") | ||
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("includeconf=relative2.conf\n") | ||
|
||
self.restart_node(0) | ||
|
||
subversion = self.nodes[0].getnetworkinfo()["subversion"] | ||
assert subversion.endswith("main; relative; relative2)/") | ||
|
||
if __name__ == '__main__': | ||
IncludeConfTest().main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a loop, otherwise only the first
-includeconf
is removed fromm_override_args
. Also update comment above accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is defined as a
std::map<std::string, std::vector<std::string>>
, which may only have one entry per key. It removes the vector itself, which may contain multiple values. (I am only showing the first for simplicity in the error output.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Either log all or nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Added loop over vector.