Test datadir specified in conf file exists #11829

meshcollider · 2017-12-05T11:28:41Z

Provoked by Nick ODell's discovery here: https://bitcoin.stackexchange.com/questions/64189/when-running-bitcoind-i-keep-getting-boostfilesystemspace-operation-not-p/64210#64210

If a custom data directory is specified using -datadir argument, its existence is checked before the conf file is loaded. But if the conf file then specifies a different non-existent datadir, that isn't tested, and results in esoteric errors like:

EXCEPTION: N5boost10filesystem16filesystem_errorE       
boost::filesystem::space: Operation not permitted

This just adds a check for the datadir existence at the end of ReadConfigFile()

promag

~~Concept ACK. Add test?~~

achow101 · 2017-12-05T16:21:17Z

I don't think the datadir should ever be specified in the bitcoin.conf file. That would imply that the conf file is in one place (the original datadir) and the actual data is elsewhere. This splits up the contents of the datadir which I don't think we should allow.

I think it would be better to throw an error if it sees the datadir option there or ignore it.

promag · 2017-12-05T16:25:42Z

Agree with @achow101, in that case we should fail if datadir is specified in .conf file?

throw an error if it sees the datadir option there

Edit: ☝️ 👍 @achow101

meshcollider · 2017-12-05T23:35:02Z

@achow101 it's already possible to split up the datadir in the same way by specifying -conf isn't it?

sipa

utACK 529b866

sipa · 2017-12-06T05:30:45Z

src/util.cpp

@@ -639,6 +639,9 @@ void ArgsManager::ReadConfigFile(const std::string& confPath)
    }
    // If datadir is changed in .conf file:
    ClearDatadirCache();
+    if (!fs::is_directory(GetDataDir(false))) {
+        throw std::runtime_error(strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", "").c_str()));


Any reason why you're once using GetDataDir(false) and once gArgs.GetArg("-datadir", "")? It's a bit confusing if they can ever differ, and if so, why they're used here.

If the datadir doesn't exist, GetDataDir returns an empty string so the argument needs to be accessed directly in that case

sipa · 2017-12-06T06:44:21Z

@achow101 It's more complicated, as it is possible to specify the config file on the command line too (which then on its turn can set the datadir). This will get even harder once we add network-specific config (#10996). I'd suggest merging this as is, and then separately thinking hard about what configurations should be supported.

meshcollider · 2017-12-06T11:21:23Z

Yeah I've wanted to have a proper think about the config file stuff for a while, since #10996 and #10267, and suggestions like whether -conf argument should be repeatable, whether there should be a separate -netconf argument, etc. otherwise it's going to get out of hand with conf, netconf, datadir, includeconf, etc.
Perhaps a topic for this weeks meeting.

promag · 2017-12-06T11:32:29Z

If a custom data directory is specified using -datadir argument, its existence is checked before the conf file is loaded. But if the conf file then specifies a different non-existent datadir, that isn't tested, and results in esoteric errors like:

If -datadir is specified in the command line, the .conf value should be ignored, because command line arguments should have precedence over .conf values. Or am I wrong?

meshcollider · 2017-12-06T12:09:53Z

@promag I believe what would happen is that the -datadir argument would cause bitcoin to look for the conf file in that specified datadir, and then if that conf file specified a different datadir, then the one in the conf file would override the command line one

promag · 2017-12-06T12:13:55Z

I always thought that command line args take precedence over .conf args.

meshcollider · 2017-12-06T12:26:22Z

That would maybe be nice in theory but in the code it only matters in which order they are read :) I think after we discuss it in the meeting I might try and rework some of this

promag · 2017-12-07T00:51:01Z

@meshcollider see above

bitcoin/src/util.cpp

Line 631 in 497d0e0

    
           // Don't overwrite existing settings so command line settings override bitcoin.conf

Anyway, utACK 529b866.

laanwj · 2017-12-07T12:22:59Z

I don't think the datadir should ever be specified in the bitcoin.conf file.

Why not? Mind you ,the conf is not bound to the data directory.

I've seen setups such as
bitcoind -conf=/etc/bitcoin.conf
with /etc/bitcoin.conf having the datadir=... directory.

laanwj · 2017-12-07T13:06:35Z

(11829)$ src/bitcoind -datadir=/tmp/testsfdsdf
Error: Specified data directory "/tmp/testsfdsdf" does not exist.
(11829)$ echo "datadir=/tmp/testsfdsdf" > /tmp/bitcoin.conf
(11829)$ src/bitcoind -conf=/tmp/bitcoin.conf
Error reading configuration file: specified data directory "/tmp/skflaskjafslkj" does not exist.

Tested ACK 529b866 (though we really need automated tests for this!)

529b866 Test datadir in conf file exists (MeshCollider) Pull request description: Provoked by Nick ODell's discovery here: https://bitcoin.stackexchange.com/questions/64189/when-running-bitcoind-i-keep-getting-boostfilesystemspace-operation-not-p/64210#64210 If a custom data directory is specified using `-datadir` argument, its existence is checked before the conf file is loaded. But if the conf file then specifies a different non-existent `datadir`, that isn't tested, and results in esoteric errors like: EXCEPTION: N5boost10filesystem16filesystem_errorE boost::filesystem::space: Operation not permitted This just adds a check for the datadir existence at the end of `ReadConfigFile()` Tree-SHA512: e488618c40aa356263f94040ae00aa4be98038abef66e8674b01032d22a5553a7fafcb8fe2d1f095865b39fb138c07b7a94415a00ef837573f92f95af065f712

promag · 2017-12-07T13:26:25Z

though we really need automated tests for this!

Agree.

meshcollider · 2017-12-07T17:23:59Z

though we really need automated tests for this!

Will write some soon :)

be9a13c Add configuration/argument testing (MeshCollider) Pull request description: Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests. Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in #11829. It also tests that command line arguments override the ones in the config file. I plan on working on a fix for #11819 / #1044 and then expanding this test with cases for that. Tree-SHA512: 97aea18c67d331db3ca3d0c99c79267cf012df67fddafc9fac63d392f5c3a6469aa14d93b5865c3bbe561461648d2485367978a77446483b8df53d1916f1c8e8

be9a13c Add configuration/argument testing (MeshCollider) Pull request description: Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests. Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in bitcoin/bitcoin#11829. It also tests that command line arguments override the ones in the config file. I plan on working on a fix for bitcoin/bitcoin#11819 / bitcoin/bitcoin#1044 and then expanding this test with cases for that. Tree-SHA512: 97aea18c67d331db3ca3d0c99c79267cf012df67fddafc9fac63d392f5c3a6469aa14d93b5865c3bbe561461648d2485367978a77446483b8df53d1916f1c8e8

529b866 Test datadir in conf file exists (MeshCollider) Pull request description: Provoked by Nick ODell's discovery here: https://bitcoin.stackexchange.com/questions/64189/when-running-bitcoind-i-keep-getting-boostfilesystemspace-operation-not-p/64210#64210 If a custom data directory is specified using `-datadir` argument, its existence is checked before the conf file is loaded. But if the conf file then specifies a different non-existent `datadir`, that isn't tested, and results in esoteric errors like: EXCEPTION: N5boost10filesystem16filesystem_errorE boost::filesystem::space: Operation not permitted This just adds a check for the datadir existence at the end of `ReadConfigFile()` Tree-SHA512: e488618c40aa356263f94040ae00aa4be98038abef66e8674b01032d22a5553a7fafcb8fe2d1f095865b39fb138c07b7a94415a00ef837573f92f95af065f712

be9a13c Add configuration/argument testing (MeshCollider) Pull request description: Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests. Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in bitcoin#11829. It also tests that command line arguments override the ones in the config file. I plan on working on a fix for bitcoin#11819 / bitcoin#1044 and then expanding this test with cases for that. Tree-SHA512: 97aea18c67d331db3ca3d0c99c79267cf012df67fddafc9fac63d392f5c3a6469aa14d93b5865c3bbe561461648d2485367978a77446483b8df53d1916f1c8e8

be9a13c Add configuration/argument testing (MeshCollider) Pull request description: Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests. Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in bitcoin/bitcoin#11829. It also tests that command line arguments override the ones in the config file. I plan on working on a fix for bitcoin/bitcoin#11819 / bitcoin/bitcoin#1044 and then expanding this test with cases for that. Tree-SHA512: 97aea18c67d331db3ca3d0c99c79267cf012df67fddafc9fac63d392f5c3a6469aa14d93b5865c3bbe561461648d2485367978a77446483b8df53d1916f1c8e8

8817d76 Add config changes to release notes (random-zebra) 988d428 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 7b72630 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 51c4aa1 [tests] Unit tests for network-specific config entries (Anthony Towns) 30f3bab ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) 1bddffe ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) ef505ea [Tests] Fix and re-activate rpc_users.py functional test (random-zebra) 0eacce7 [RPC] Add rpcauth startup flag for multiple RPC users (random-zebra) e79ff3c [Tests] Fix and re-activate feature_config_args functional test (random-zebra) 3fcaaa4 Replace cookie auth in tests (random-zebra) b907f33 rpc: Generate auth cookie in hex instead of base64 (random-zebra) bf443a9 [tests] Use regtest section in functional tests configs (Anthony Towns) 7857bcd [tests] Unit tests for config file sections (Anthony Towns) 4cf2ee6 ArgsManager: support config file sections (Anthony Towns) 2658771 ArgsManager: drop m_negated_args (Anthony Towns) 4ee69b5 ArgsManager: keep command line and config file arguments separate (random-zebra) 4f416b8 [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) 5d1200b [tests] Check GetChainName works with config entries (Anthony Towns) 157da7c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 23a4633 ReadConfigStream: assume the stream is good (Anthony Towns) 56ea59e Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) a3438a2 Test datadir in conf file exists (MeshCollider) 459c4ad [tests] Add unit tests for GetChainName (Anthony Towns) 0ab3e99 Move ChainNameFromCommandLine into ArgsManager, rename to GetChainName (Anthony Towns) 1ea7ce6 Globals: Decouple GetConfigFile and ReadConfigFile from global mapArgs (random-zebra) Pull request description: It is now possible for a single configuration file to set different options for different networks. This is done by using sections or by prefixing the option with the network, such as: ``` main.uacomment=pivx test.uacomment=pivx-testnet regtest.uacomment=regtest [main] mempoolsize=300 [test] mempoolsize=100 [regtest] mempoolsize=20 ``` The `addnode=`, `connect=`, `port=`, `bind=`, `rpcport=`, `rpcbind=`, and `wallet=` options will only apply to mainnet when specified in the configuration file, unless a network is specified. Also - fix cookie-based authentication for the functional tests, and re-enable `feature_config_args.py` - add `-rpcauth` startup flag, for multiple RPC users, and re-enable `rpc_users.py`. Backports relevant commits from: - bitcoin#8856 Globals: Decouple GetConfigFile and ReadConfigFile from global mapArgs - bitcoin#11829 Test datadir specified in conf file exists - bitcoin#12878 Config handling refactoring in preparation for network-specific sections - bitcoin#11862 Network specific conf sections - bitcoin#10533 [tests] Use cookie auth instead of rpcuser and rpcpassword - bitcoin#8858 Generate auth cookie in hex instead of base64 ACKs for top commit: Fuzzbawls: Tested ACK 8817d76 furszy: utACK 8817d76 Tree-SHA512: 0d23dbf3d254b186a5378576601cf43f8322abe036b0b135a39b22e13ffa2e299cb1323160d87c7d8284e6aaa229c47f54c8a3a3ebf07cc298e644fb8bd69dc0

be9a13c Add configuration/argument testing (MeshCollider) Pull request description: Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests. Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in bitcoin#11829. It also tests that command line arguments override the ones in the config file. I plan on working on a fix for bitcoin#11819 / bitcoin#1044 and then expanding this test with cases for that. Tree-SHA512: 97aea18c67d331db3ca3d0c99c79267cf012df67fddafc9fac63d392f5c3a6469aa14d93b5865c3bbe561461648d2485367978a77446483b8df53d1916f1c8e8

Test datadir in conf file exists

529b866

fanquake added the Utils/log/libs label Dec 5, 2017

promag reviewed Dec 5, 2017

View reviewed changes

sipa reviewed Dec 6, 2017

View reviewed changes

sipa mentioned this pull request Dec 6, 2017

When running bitcoind I keep getting - boost::filesystem::space: Operation not permitted #11825

Closed

laanwj merged commit 529b866 into bitcoin:master Dec 7, 2017

meshcollider deleted the 201712_datadir_crash branch December 7, 2017 17:23

This was referenced Dec 12, 2017

Cleanups + nit fixes for walletdir PR #11726

Merged

Add configuration file/argument testing #11883

Merged

dgenr8 mentioned this pull request May 15, 2018

read config file before checking for existence of datadir bitcoinxt/bitcoinxt#423

Merged

random-zebra mentioned this pull request Apr 19, 2021

[Core] Network specific configuration sections PIVX-Project/PIVX#2324

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test datadir specified in conf file exists #11829

Test datadir specified in conf file exists #11829

meshcollider commented Dec 5, 2017

promag left a comment •

edited

achow101 commented Dec 5, 2017 •

edited

promag commented Dec 5, 2017 •

edited

meshcollider commented Dec 5, 2017

sipa left a comment

sipa Dec 6, 2017

meshcollider Dec 6, 2017 •

edited

sipa commented Dec 6, 2017

meshcollider commented Dec 6, 2017 •

edited

promag commented Dec 6, 2017 •

edited

meshcollider commented Dec 6, 2017

promag commented Dec 6, 2017

meshcollider commented Dec 6, 2017

promag commented Dec 7, 2017

laanwj commented Dec 7, 2017

laanwj commented Dec 7, 2017

promag commented Dec 7, 2017

meshcollider commented Dec 7, 2017

Test datadir specified in conf file exists #11829

Test datadir specified in conf file exists #11829

Conversation

meshcollider commented Dec 5, 2017

promag left a comment • edited

Choose a reason for hiding this comment

achow101 commented Dec 5, 2017 • edited

promag commented Dec 5, 2017 • edited

meshcollider commented Dec 5, 2017

sipa left a comment

Choose a reason for hiding this comment

sipa Dec 6, 2017

Choose a reason for hiding this comment

meshcollider Dec 6, 2017 • edited

Choose a reason for hiding this comment

sipa commented Dec 6, 2017

meshcollider commented Dec 6, 2017 • edited

promag commented Dec 6, 2017 • edited

meshcollider commented Dec 6, 2017

promag commented Dec 6, 2017

meshcollider commented Dec 6, 2017

promag commented Dec 7, 2017

laanwj commented Dec 7, 2017

laanwj commented Dec 7, 2017

promag commented Dec 7, 2017

meshcollider commented Dec 7, 2017

promag left a comment •

edited

achow101 commented Dec 5, 2017 •

edited

promag commented Dec 5, 2017 •

edited

meshcollider Dec 6, 2017 •

edited

meshcollider commented Dec 6, 2017 •

edited

promag commented Dec 6, 2017 •

edited