Test datadir specified in conf file exists #11829

pull meshcollider wants to merge 1 commits into bitcoin:master from meshcollider:201712_datadir_crash changing 1 files +3 −0
  1. meshcollider commented at 11:28 AM on December 5, 2017: contributor

    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()

  2. Test datadir in conf file exists 529b866759
  3. fanquake added the label Utils/log/libs on Dec 5, 2017
  4. promag commented at 2:24 PM on December 5, 2017: member

    Concept ACK. Add test?

  5. achow101 commented at 4:21 PM on December 5, 2017: member

    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.

  6. promag commented at 4:25 PM on December 5, 2017: member

    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

  7. meshcollider commented at 11:35 PM on December 5, 2017: contributor

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

  8. in src/util.cpp:643 in 529b866759 
     638 | @@ -639,6 +639,9 @@ void ArgsManager::ReadConfigFile(const std::string& confPath)
 639 |      }
 640 |      // If datadir is changed in .conf file:
 641 |      ClearDatadirCache();
 642 | +    if (!fs::is_directory(GetDataDir(false))) {
 643 | +        throw std::runtime_error(strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", "").c_str()));
    sipa commented at 5:30 AM on December 6, 2017:

    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.

    meshcollider commented at 11:15 AM on December 6, 2017:

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

  9. sipa commented at 5:31 AM on December 6, 2017: member

    utACK 529b8667599ad74f6dae639b889d22e907353070

  10. sipa commented at 6:44 AM on December 6, 2017: member

    @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.

  11. meshcollider commented at 11:21 AM on December 6, 2017: contributor

    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.

  12. promag commented at 11:32 AM on December 6, 2017: member

    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?

  13. meshcollider commented at 12:09 PM on December 6, 2017: contributor

    @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

  14. promag commented at 12:13 PM on December 6, 2017: member

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

  15. meshcollider commented at 12:26 PM on December 6, 2017: contributor

    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

  16. promag commented at 12:51 AM on December 7, 2017: member

    @MeshCollider see above https://github.com/bitcoin/bitcoin/blob/497d0e014cc79d46531d570e74e4aeae72db602d/src/util.cpp#L631

    Anyway, utACK 529b866.

  17. laanwj commented at 12:22 PM on December 7, 2017: member

    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.

  18. laanwj commented at 1:06 PM on December 7, 2017: member 
    (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!)

  19. laanwj merged this on Dec 7, 2017
  20. laanwj closed this on Dec 7, 2017
  21. laanwj referenced this in commit 7630a1fe9a on Dec 7, 2017
  22. promag commented at 1:26 PM on December 7, 2017: member

    though we really need automated tests for this!

    Agree.

  23. meshcollider deleted the branch on Dec 7, 2017
  24. meshcollider commented at 5:23 PM on December 7, 2017: contributor

    though we really need automated tests for this!

    Will write some soon :)

  25. laanwj referenced this in commit cfd99ddc3c on Dec 20, 2017
  26. virtload referenced this in commit 6ad76a91f9 on Apr 4, 2018
  27. PastaPastaPasta referenced this in commit e56fe1b657 on Jan 17, 2020
  28. PastaPastaPasta referenced this in commit faee60c2b9 on Jan 22, 2020
  29. PastaPastaPasta referenced this in commit ecb659bdcd on Jan 22, 2020
  30. PastaPastaPasta referenced this in commit 0de57009a6 on Jan 29, 2020
  31. PastaPastaPasta referenced this in commit 555a0498ba on Jan 29, 2020
  32. PastaPastaPasta referenced this in commit e98c1f2d5a on Jan 29, 2020
  33. PastaPastaPasta referenced this in commit 1a754b5d7b on Jan 31, 2020
  34. PastaPastaPasta referenced this in commit bdf1e71125 on Feb 13, 2020
  35. PastaPastaPasta referenced this in commit d7dec0fbfb on Feb 27, 2020
  36. PastaPastaPasta referenced this in commit 262bac5213 on Feb 27, 2020
  37. PastaPastaPasta referenced this in commit df30971371 on Feb 27, 2020
  38. akshaynexus referenced this in commit 3180832e3f on May 6, 2020
  39. ckti referenced this in commit 9af2d47de8 on Mar 28, 2021
  40. ckti referenced this in commit 36fc29566d on Mar 28, 2021
  41. furszy referenced this in commit 0255df35a6 on Apr 26, 2021
  42. gades referenced this in commit 98834cbae2 on Jun 30, 2021
  43. DrahtBot locked this on Sep 8, 2021
Contributors
meshcolliderpromagachow101sipalaanwj
Labels
Utils/log/libs
Linked (view graph)
#11883 Add configuration file/argument testing
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me