New -includeconf argument for including external configuration files by kallewoof · Pull Request #10267 · bitcoin/bitcoin

kallewoof · 2017-04-24T01:09:49Z

Done:

adds -includeconf=<path>, where <path> is relative to datadir or to the path of the file being read, if in a file
protects against circular includes
updates help docs

- ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~

jonasschnelli · 2017-04-24T06:12:08Z

Yeah. Why not. This can be useful.

I would recommend to use -addconf= (otherwise user may think it replaces the bitcoin.conf configuration file).
If I follow GetConfigFile() correctly, you can also use absolut paths, right?

NicolasDorier · 2017-04-24T07:15:46Z

Why not making the existing -config a repeatable argument.

kallewoof · 2017-04-24T08:00:00Z

@jonasschnelli Good point - will switch to -addconf=. Yes, you can use absolute paths. My worry above is for when a user presumes the path is relative to the config file when it is in fact not.

@NicolasDorier I think -config simply tells what name to use and defaults to bitcoin.conf -- it doesn't actually load the file. This feature lets you load other files arbitrarily from within bitcoin.conf.

kallewoof · 2017-04-24T09:35:45Z

Unsquashed history: 1 → 2 → 3⊱1 → 4⊱2

laanwj · 2017-04-24T11:53:18Z

Concept ACK

My worry above is for when a user presumes the path is relative to the config file when it is in fact not.

Yes, making it relative to the data directory is a good choice. I think we should handle all relative paths in bitcoind that way.

Why not making the existing -config a repeatable argument.

That was also my first thought, but it may just be confusing as it changes the meaning of the option slightly. It's possible that some setups already use multiple -conf options, and rely on the overriding behavior.

So I'm good with making it an explicit option. Another suggestion for the name would be -includeconf.

jtimon · 2017-04-24T16:50:28Z

Concept ACK. Don't care much about the name, but what about -extraconf ?

kallewoof · 2017-04-25T01:47:24Z

From the given suggestions I think includeconf is the most clear so I'll switch to that.

@laanwj:

Yes, making it relative to the data directory is a good choice. I think we should handle all relative paths in bitcoind that way.

To clarify, you mean that the relative path inside /dir/file.conf should be /dir/, not [bitcoin datadir], right? It will require some lines of code I bet but I think that makes sense too.

kallewoof · 2017-04-25T03:00:18Z

The includeconf feature now defines the path relative to the file being read, if any. For command line, it is [datadir], for /dir/abc.conf it is /dir/. I tested this with

src/testreadconfig/bitcoin.conf: [...] includeconf=../global.conf
src/global.conf: includeconf=secrets.conf
src/secrets.conf: rpcpassword=foo

with bitcoind -datadir=testreadconfig. Ensured bitcoin-cli with password foo worked and password bar did not.

[...]: → 5⊱1 → 6⊱2

laanwj · 2017-04-25T05:14:44Z

To clarify, you mean that the relative path inside /dir/file.conf should be /dir/, not [bitcoin datadir], right? It > will require some lines of code I bet but I think that makes sense too.

Yes, seems good to me too. So it's like C's include "" - I wasn't thinking about relative includes in other includes.

kallewoof · 2017-04-25T07:34:47Z

To clarify, the code now does what @laanwj suggested.

laanwj · 2017-05-01T11:59:53Z

I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.

Some ideas:

RPC test that creates a tree of bitcoin config files including each other beneath the data directory
Starts a node w/ -includeconf=<path>
Then interrogate node over RPC to verify the files got included, in the right order

To achieve the latter the option uacomment= is useful, as these will be added to an array, then querying getnetworkinfo to see if the (...) part of the subversion matches expected content and order.

jnewbery · 2017-05-01T21:43:21Z

@laanwj's suggested test method seems sensible. I'm happy to review that or lend a hand implementing it. Feel free to reach me on IRC.

kallewoof · 2017-05-02T01:28:44Z

Thanks for the suggestion! I added a test that checks for load order and ensures circular include is guarded against. @jnewbery review would be wonderful :)

Edit: If anyone has ideas why travis is failing I'd appreciate it. It works fine on all the machines I test it on locally (mac, linux).

jnewbery · 2017-05-02T15:33:53Z

If anyone has ideas why travis is failing I'd appreciate it

You've made ReadConfigFile() recursive (through ProcessSetting()). ReadConfigFile() locks cs_args, and then at the end calls ClearDatadirCache(), which locks csPathCached. That means that the bottom-most ReadConfigFile() locks csPathCached while cs_args is still held.

There's already a function that locks in the other order: GetDataDir() locks csPathCached and then locks cs_args (in its call to IsArgSet()).

If those two functions are called in different threads, we'd have a deadlock.

There's a CPP_FLAG option that checks lock ordering CPPFLAGS=-DDEBUG_LOCKORDER, which is used in Travis build 5. That's why that build is failing. You can repro locally by running configure with that option.

You can fix this by not locking cs_args recursively.

kallewoof · 2017-05-08T04:02:22Z

@jnewbery Thanks a lot for the explanation! I should've paid closer attention to locks considering the added recursiveness.

97ee63b fixes this by moving the conditionally-locked code into a new ReadConfigStream function which is called with locking/clearing in one case and without in the other, based on a bool lockAndClear added to ReadConfigFile.

(Also had to tweak tests a tiny bit; 8fb6511.)

jnewbery · 2017-05-15T19:57:19Z

I think you've introduced a subtle bug here. If -datadir is configured in one of the additional config files, then the datadir cache won't be cleared, which means that bitcoind will continue to use the old datadir.

I think you should try to not make ReadConfigFile recursive. For me, it would be acceptable to only allow one level of redirection here (ie the "base" config file can specify -includeconf config file, but those included config files cannot themselves include other config files). I think that would be a simpler model and would remove a whole bunch of potential bugs (circular references, blowing the stack through too many -includeconf redirects, etc).

kallewoof · 2017-05-16T00:54:23Z

@jnewbery Hm, no the datadir cache is cleared after any recursions happen, which means it is always cleared, just not directly after the config file has been parsed. There are two cases:

ParseParameters (util.cpp:407 called from bitcoind.cpp:75) -- this will work as normal and does not require cache clearing.
Nested ReadConfigs from the initial bitcoin.conf file: bitcoind:107 calls ReadConfigFile with the lockAndClear flag set; recursion then happens in ProcessSetting (L401) via ReadConfigStream (L622) with lockAndClear unset. Eventually this gets back to original caller which leaves ReadConfigStream and gets to util.cpp:649 which clears the datadir cache.
Or am I missing something?

As for forbidding multiple levels of recursion, I think the value outweighs the issues personally (and I addressed circular refs I believe), but if people think it's not worth it I'll restrict it to one include.

jnewbery · 2017-05-16T15:00:40Z

@kallewoof yes you're right. datadir cache is cleared after all files are read. My mistake.

I still don't like the recursion and the fact that there can be multiple levels of imports. It means there are more edge cases and unexpected behaviour. For example, if -includeconf is included as a command line parameter, then the includeconf file is read before the regular conf file, and so takes precedence. If an includeconf line is included in the regular conf file, then it is read during the regular config file, and which settings are taken from the regular conf file and which are taken from the includeconf file depend on the ordering of settings in the regular conf file.

The new warnOnFailure and lockAndClear bool arguments to ReadConfigFile() seem pretty strange to me. They're only used when ReadConfigFile() is being called recursively, and they control a large chunk of the behaviour within ReadConfigFile(). That's a clue to me that maybe the functionality isn't split up correctly - perhaps the locking/clearing should be in an outer function which calls an inner function for each of the config files?

Finally, you've introduced a new crash bug. If -conf or -includeconf don't refer to a valid file, the bitcoind will crash on startup. Here's the backtrace:

Core was generated by `bitcoind -datadir=/tmp/user/1000/test6p7xn_xt/856/node0 -server -keypool=1 -dis'.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007f3b77df0428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00007f3b77df0428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007f3b77df202a in __GI_abort () at abort.c:89
#2  0x00007f3b7873284d in __gnu_cxx::__verbose_terminate_handler() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007f3b787306b6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007f3b78730701 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007f3b78730919 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007f3b79f29f82 in boost::filesystem::detail::canonical(boost::filesystem::path const&, boost::filesystem::path const&, boost::system::error_code*) () from /usr/lib/x86_64-linux-gnu/libboost_filesystem.so.1.58.0
#7  0x000055e5c58b2ae2 in boost::filesystem::canonical (base=..., p=...) at /usr/include/boost/filesystem/operations.hpp:459
#8  GetConfigFile (confPath="global.conf", relativePath="") at util.cpp:603
#9  0x000055e5c58b4353 in ArgsManager::ProcessSetting (this=this@entry=0x55e5c5db1fa0 <gArgs>, strKey="-includeconf", strValue="global.conf", relativePath="") at util.cpp:386
#10 0x000055e5c58b487d in ArgsManager::ParseParameters (this=0x55e5c5db1fa0 <gArgs>, argc=<optimized out>, argv=<optimized out>) at util.cpp:424
#11 0x000055e5c5653d4a in ParseParameters (argv=0x7fff5492e618, argc=14) at util.h:263
#12 AppInit (argc=14, argv=0x7fff5492e618) at bitcoind.cpp:75
#13 0x000055e5c5648bef in main (argc=14, argv=0x7fff5492e618) at bitcoind.cpp:196
(gdb) quit

If I use an invalid filename for -conf on master. I don't see this crash.

jnewbery · 2018-04-30T20:54:36Z

I'd prefer to not to have special stderr checking for this test. I have a PR open here: #12755 which adds stderr checking to the test framework.

I've also prepared a branch here: https://github.com/bitcoin/bitcoin/compare/master...jnewbery:pr10267.3?expand=1 which rebases this PR on #12755 and adds explicit stderr checking.

Would you mind either:

removing the stderr checking from this PR entirely (and I'll add it as part of [tests] Better stderr testing #12755); or
resetting this PR to https://github.com/jnewbery/bitcoin/tree/pr10267.3 (which would mean that this PR now depends on [tests] Better stderr testing #12755)

kallewoof · 2018-05-01T04:31:26Z

@jnewbery Wouldn't it be simpler to fix this in a follow-up PR once #12755 is merged?

jnewbery

Tested ACK 6d5605517b477538f0447d41fe0bc734da27d10c. A couple of nits in the new test code, but nothing that should prevent merge.

@jnewbery Wouldn't it be simpler to fix this in a follow-up PR once #12755 is merged?

Not 100% sure what you mean by 'fix this', but I'm guessing you mean merge this as is and then remove the specific stderr checking code from this test when the generic stderr checking is added in #12755? Sure - that's fine, as long as you don't mind reviewing #12755 for me :)

jnewbery · 2018-05-01T13:42:07Z

test/functional/feature_includeconf.py

nit: PEP8 import ordering please (std library first, then local application imports)

jnewbery · 2018-05-01T13:44:10Z

test/functional/feature_includeconf.py

micronit: all other tests have set_test_params() as the first method. I think that makes more logical sense since that method is called by __init__(). Do you mind doing the same here?

kallewoof · 2018-05-02T05:27:59Z

@jnewbery Thanks for review! Addressed your nits.

kallewoof · 2018-05-02T05:31:26Z

@jnewbery I'm a little confused. Should I close this PR in favor of #12755? I thought #12755 only added the error stuff, but it includes this PR as well now.

jnewbery · 2018-05-03T19:48:28Z

Tested ACK 25b7ab9.

I'm a little confused. Should I close this PR in favor of #12755? I thought #12755 only added the error stuff, but it includes this PR as well now.

Sorry - I haven't been very clear in my updates. Now that this PR includes stderr testing, it conflicts with #12755. My initial suggestions were to remove the stderr testing from this test, or rebase on top of #12755. But now I think it would make more sense to rebase #12755 on this, since this PR has been around for so long and should almost be ready for merge.

I've rebased #12755 on this. As soon as this PR is merged, #12755 will be ready to go in.

jnewbery · 2018-05-08T19:11:59Z

I think this is very nearly ready for merge. Are previous reviewers @laanwj, @ajtowns @promag, @ryanofsky, @MarcoFalke @meshcollider, @jonasschnelli able to re-review this?

laanwj · 2018-05-08T19:40:06Z

utACK 25b7ab9

promag · 2018-05-08T21:20:31Z

utACK 25b7ab9.

I suggest to add a warning if -includeconf is used in an included file (follow up PR).

…uration files 25b7ab9 doc: Add release notes for -includeconf (Karl-Johan Alm) 0f0badd test: Test includeconf parameter. (Karl-Johan Alm) 629ff8c -includeconf=<path> support in config handler, for including external configuration files (Karl-Johan Alm) Pull request description: Fixes: #10071. Done: - adds `-includeconf=<path>`, where `<path>` is relative to `datadir` or to the path of the file being read, if in a file - protects against circular includes - updates help docs ~~~Thoughts:~~~ - ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~ Tree-SHA512: cb31f1b2f69fbc0890d264948eb2e501ac05cf12f5e06a5942f9c1539eb15ea8dc3cae817f4073aecb2fcc21d0386747f14f89d990772003a76e2a6d25642553

beee49b [tests] Allow stderr to be tested against specified string (John Newbery) e503671 [Tests] Use LIBC_FATAL_STDERR_=1 in tests (John Newbery) c22ce8a [Tests] Write stdout/stderr to datadir instead of temp file. (John Newbery) Pull request description: **Due to a merge conflict, this is now based on #10267. Please review that PR first!** Subset of #12379 now that parts of that PR have been merged. #12362 was only observed when running the functional tests locally because: - by defatul libc logs to `/dev/tty` instead of stderr - the functional tests only check for substring inclusion in stderr when we're expecting bitcoind to fail. This PR tightens our checking of stderr and will cause tests to fail if there is any unexpected message in stderr: - commit *Write stdout/stderr to datadir instead of temp file* writes stderr to a file in the datadir instead of a temporary file. This helps with debugging in the case of failure. - commit *Use LIBC_FATAL_STDERR=1 in tests* ensures that libc failures are logged to stderr instead of the terminal. commit *Assert that bitcoind stdout is empty on shutdown* asserts that stderr is empty on bitcoind shutdown. Tree-SHA512: 21111030e667b3b686f2a7625c2b625ebcfb6998e1cccb4f3932e8b5d21fb514b19a73ac971595d049343430e9a63155986a7f5648cad55b8f36f3c58b1c7048

practicalswift · 2018-05-12T20:30:42Z

When working on the locking annotations (see #13126) I noticed that ArgsManager::ReadConfigFiles() introduced in this PR accesses m_override_args without first locking cs_args.

The PR #13126 adds the correct locking. FWIW Travis will catch this type of cs_args locking violations when #13126 is merged :-)

@jnewbery

2352aa9 test: Ensure that recursive -includeconf produces appropriate warnings (Karl-Johan Alm) c5bcc7d util: warn about recursive -includeconf arguments in configuration files (Karl-Johan Alm) Pull request description: This is a follow-up PR to #10267, and addresses #10267 (comment). ~~I am adding extra work for @jnewbery in #12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.~~ Tree-SHA512: 87f0c32436b70424e33616ffb88d7cb699f90d6a583a10237e224b28fc936d6a9df95536c8c52ee8546b3942da92b2a357e61bf87e00d1462bc10d46d3bee352

maflcko

Post-merge ACK 25b7ab9

See #13367 to fix my nit about the errors.

maflcko · 2018-06-01T20:13:27Z

test/functional/feature_includeconf.py

+            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')


nit: should probably be an (init)error, since it is clearly an invalid command line

maflcko · 2018-06-01T20:14:43Z

src/util.cpp

+                    ReadConfigStream(include_config);
+                    LogPrintf("Included configuration file %s\n", to_include.c_str());
+                } else {
+                    fprintf(stderr, "Failed to include configuration file %s\n", to_include.c_str());


nit: return false on error?

Summary: - adds -includeconf=<path>, where <path> is relative to datadir or to the path of the file being read, if in a file - protects against circular includes - updates help docs Backport of core PR10267 https://github.com/bitcoin/bitcoin/pull/10267/files Includes a fix from PR13126 bitcoin/bitcoin#10267 (comment) Completes T541 Progresses towards T652 Test Plan: make check ./test/functional/test_runner.py feature_includeconf Reviewers: #bitcoin_abc, deadalnix, markblundeberg Reviewed By: #bitcoin_abc, markblundeberg Subscribers: markblundeberg Maniphest Tasks: T541 Differential Revision: https://reviews.bitcoinabc.org/D3035

@jnewbery

Summary: 2352aa9 test: Ensure that recursive -includeconf produces appropriate warnings (Karl-Johan Alm) c5bcc7d util: warn about recursive -includeconf arguments in configuration files (Karl-Johan Alm) Pull request description: This is a follow-up PR to #10267, and addresses bitcoin/bitcoin#10267 (comment). ~~I am adding extra work for @jnewbery in #12755 here -- maybe I should just rebase on top of that, but not sure what the appropriate approach is here.~~ Tree-SHA512: 87f0c32436b70424e33616ffb88d7cb699f90d6a583a10237e224b28fc936d6a9df95536c8c52ee8546b3942da92b2a357e61bf87e00d1462bc10d46d3bee352 Backport of Core PR13197 bitcoin/bitcoin#13197 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4118

kallewoof force-pushed the feature-config-readconfig branch from 1bd0ee1 to fe8850e Compare April 24, 2017 01:15

kallewoof force-pushed the feature-config-readconfig branch from fe8850e to 93818ed Compare April 24, 2017 09:33

kallewoof force-pushed the feature-config-readconfig branch from 93818ed to 939087f Compare April 25, 2017 02:53

kallewoof changed the title ~~New -readconfig argument for including external configuration files~~ New -includeconf argument for including external configuration files Apr 25, 2017

kallewoof force-pushed the feature-config-readconfig branch 2 times, most recently from 75a36ca to 5d1e823 Compare April 25, 2017 06:32

laanwj added the RPC/REST/ZMQ label Apr 25, 2017

kallewoof force-pushed the feature-config-readconfig branch 2 times, most recently from a3a8f4a to 3499187 Compare May 2, 2017 01:58

kallewoof force-pushed the feature-config-readconfig branch 2 times, most recently from 2317d90 to 8fb6511 Compare May 8, 2017 04:00

kallewoof force-pushed the feature-config-readconfig branch from 8fb6511 to 8069dbf Compare May 16, 2017 02:27

kallewoof force-pushed the feature-config-readconfig branch from 8c06988 to 6d56055 Compare April 26, 2018 03:46

jnewbery reviewed May 1, 2018

View reviewed changes

jnewbery mentioned this pull request May 1, 2018

[tests] Better stderr testing #12755

Merged

kallewoof added 2 commits May 2, 2018 14:26

test: Test includeconf parameter.

0f0badd

doc: Add release notes for -includeconf

25b7ab9

kallewoof force-pushed the feature-config-readconfig branch from 6d56055 to 25b7ab9 Compare May 2, 2018 05:26

laanwj merged commit 25b7ab9 into bitcoin:master May 9, 2018

kallewoof deleted the feature-config-readconfig branch May 9, 2018 05:42

kallewoof mentioned this pull request May 9, 2018

util: warn about ignored recursive -includeconf calls #13197

Merged

jnewbery mentioned this pull request May 9, 2018

Per-network bitcoin.conf files #9374

Closed

Bushstar mentioned this pull request May 10, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#328

Merged

practicalswift mentioned this pull request May 12, 2018

util: Add Clang thread safety annotations for variables guarded by cs_args #13126

Merged

maflcko reviewed Jun 1, 2018

View reviewed changes

This was referenced Sep 6, 2018

[tests] Better stderr testing chaincoin/chaincoin#311

Merged

util: warn about ignored recursive -includeconf calls chaincoin/chaincoin#333

Merged

Conversation

kallewoof commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Apr 24, 2017

Uh oh!

NicolasDorier commented Apr 24, 2017

Uh oh!

kallewoof commented Apr 24, 2017

Uh oh!

kallewoof commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 24, 2017

Uh oh!

jtimon commented Apr 24, 2017

Uh oh!

kallewoof commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Apr 25, 2017

Uh oh!

laanwj commented May 1, 2017

Uh oh!

jnewbery commented May 1, 2017

Uh oh!

kallewoof commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 2, 2017

Uh oh!

kallewoof commented May 8, 2017

Uh oh!

jnewbery commented May 15, 2017

Uh oh!

kallewoof commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 16, 2017

Uh oh!

jnewbery commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented May 1, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery May 1, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery May 1, 2018

Choose a reason for hiding this comment

Uh oh!

kallewoof commented May 2, 2018

Uh oh!

kallewoof commented May 2, 2018

Uh oh!

jnewbery commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 8, 2018

Uh oh!

laanwj commented May 8, 2018 via email

Uh oh!

promag commented May 8, 2018

Uh oh!

practicalswift commented May 12, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 1, 2018

Choose a reason for hiding this comment

kallewoof commented Apr 24, 2017 •

edited

Loading

kallewoof commented Apr 24, 2017 •

edited

Loading

kallewoof commented Apr 25, 2017 •

edited

Loading

kallewoof commented Apr 25, 2017 •

edited

Loading

laanwj commented Apr 25, 2017 •

edited

Loading

kallewoof commented May 2, 2017 •

edited

Loading

kallewoof commented May 16, 2017 •

edited

Loading

jnewbery commented Apr 30, 2018 •

edited

Loading

jnewbery commented May 3, 2018 •

edited

Loading