Skip to content
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

Check for datadir after the config files were read #13621

Closed

Conversation

Projects
None yet
6 participants
@Flowdalic
Copy link
Contributor

commented Jul 10, 2018

If we first check for datadir existence and then read the config file
we bail out in situations where the default datadir is invalid,
e.g. because of an non-existing HOME, but the datadir specified in
config files is valid. We should not do that, as the user may specified a
valid datadir in the config file and expects the setting to take effect.

See for example #12255 (comment) where users hit this.

@fanquake fanquake changed the title Check for datadir after the config files where read Check for datadir after the config files were read Jul 10, 2018

@Flowdalic Flowdalic force-pushed the Flowdalic:init-swap-datadir-readconf branch 2 times, most recently Jul 10, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

Doesn't the location of the default config file depend on the datadir?

@Flowdalic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

Doesn't the location of the default config file depend on the datadir?

Yes, BITCOIN_CONF_FILENAME, which is set to bitcoin.conf is feed into AbsPathForConfigVal() which uses the datadir setting. However, if no datadir is explicitly set it defaults to "". I think that this means that bitcoin.conf is opened relative to the processe's working directory.

It appears that you have a specific problem in mind, which I don't see. Care to elaborate?

@MarcoFalke

This comment has been minimized.

@AtsukiTak

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

Now the second if clause seems meaningless.

https://github.com/Flowdalic/bitcoin/blob/94a038b8694229c0153ede48a53701424ca51e38/src/bitcoind.cpp#L98-L102

Because it already checked in ReadConfigFiles.

bitcoin/src/util.cpp

Lines 909 to 912 in 4a7e64f

if (!fs::is_directory(GetDataDir(false))) {
error = strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", "").c_str());
return false;
}

@Flowdalic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

Right, the check can get probably removed.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14409 (utils and libraries: Make 'blocksdir' always net specific by hebasto)
  • #14324 (qa: Run more tests with wallet disabled by MarcoFalke)
  • #13878 (utils: Add fstream wrapper to allow to pass unicode filename on Windows by ken2812221)
  • #13746 (-masterdatadir for datadir bootstrapping by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Flowdalic Flowdalic force-pushed the Flowdalic:init-swap-datadir-readconf branch 6 times, most recently Jul 19, 2018

Flowdalic added some commits Jul 19, 2018

Do not create datadir when calling GetConfigFile
As the real datadir may be set in the configuration file, this would
case the default datadir to get created.
Do not fallback to empy path in GetDataDir()
If datadir is not a directory, do not fallback to empty path as this would shadow the original path set in the config file. Consider

    bitcoin.conf:
    datadir=/does/not/exist

and bitcoind started with "-conf=bitcoin.conf", which would previusly yield

    Error: Specified data directory "" does not exist.

when it should state

    Error: Specified data directory "/does/not/exist" does not exist.
Check for datadir after the config files were read
If we first check for datadir existence and then read the config file
we bail out in situations where the default datadir is invalid,
e.g. because of an non-existing HOME, but the datadir specified in
config files is valid. We should not do that, as the user specified a
valid datadir and expects the setting to take effect.

Also remove datadir check in ReadConfigFiles() which now became
superfluous: We check for the datadir in AppInit() already, there is
no need to check also in ReadConfigFiles().

Furthermore re-enable the disabled tests in feature_config_args.py,
which where disabled in fabe28a ("qa: Temporarily disable test
that reads the default datadir location"), as those tests do not touch
"$HOME/.bitcoin" any more.

@Flowdalic Flowdalic force-pushed the Flowdalic:init-swap-datadir-readconf branch to b6a43df Jul 19, 2018

@Flowdalic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Rebased. Also fixed the cause and re-enabled the tests which were disabled in #13687 with fabe28a.

I had a hard time working on the argument and configuration file parsing code. It is a little bit spagettish and some parts are messy.

I used default argument values in order to optionally change GetDataDir() to not create the data directory, which, I assumed, made the change less intrusive. But I am happy to change it towards a GetDataDir() / GetDataDirAndCreateIfNecessary(). I am undecied which of those two approaches would be better.

@DrahtBot DrahtBot removed the Needs rebase label Jul 20, 2018

if (!gArgs.ReadConfigFiles(error, true)) {
fprintf(stderr, "Error reading configuration file: %s\n", error.c_str());
return EXIT_FAILURE;
}
if (!fs::is_directory(GetDataDir(false))) {

This comment has been minimized.

Copy link
@AtsukiTak

AtsukiTak Jul 20, 2018

Contributor

It seems never fail unless user specify unpermitted path.

This comment has been minimized.

Copy link
@Flowdalic

Flowdalic Jul 21, 2018

Author Contributor

It fails if the specified path is not a directory. I think that is sensible and was already previous behavior. Or do have a specific change request?

This comment has been minimized.

Copy link
@AtsukiTak

AtsukiTak Jul 21, 2018

Contributor

If I specify a file, boost::filesystem::create_directory throw an error.
If I specify a non-exist path, a new directory is created and no error happens.

I think you just missed second argument; GetDataDir(false, false) might be what you want.

This comment has been minimized.

Copy link
@AtsukiTak

AtsukiTak Jul 21, 2018

Contributor

Sorry for my poor English. Please feel free to ask me if my English is not clear.

@AtsukiTak

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

Nice work :)
But this PR seems to contains lots of changes for me such as

  • Check for datadir after the config files were read
  • Create directory if user specify non exist directory
  • Do not fallback to empy path in GetDataDir()

I think you should leave only first one, and move rests to another PR, and then discuss about them.

@Flowdalic

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2018

I think you should leave only first one

Well, those commits depend on each other: For example I wouldn't be able to re-enable the relevant tests of "Check for datadir after the config files were read" without the previous commits.

I don't feel like that this patch series is worth splitting up, as it is pretty small, but I am happy create single PRs for each commit one after another, if there is a consensus that this would be favorable.

@AtsukiTak

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

I don't feel like that this patch series is worth splitting up, as it is pretty small

I see. So please don't care about that. That was just newbie's nonsense comment.

@leishman

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

I don't think our PRs conflict, but just wanted to make you aware of a minor change I'm putting in the logging behavior: #14057

I'm also working on logic to create a bitcoin.conf.example file: #13761

@Flowdalic

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

Pinging @MarcoFalke since this PR re-enables tests he disabled in fabe28a (#13687). Would be great to hear your thoughts on this.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

I haven't looked at the most recent code, but I still think that the location of the default config file depends on the datadir? See https://dev.visucore.com/bitcoin/doxygen/class_args_manager.html#aec848fd54ace0df3df48dac446feb7d2 which calls GetDataDir.

So you are changing how and what config files are read. Not sure if we want this behavior change without discussion first.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Also, you modified how bitcoind and bitcoin-cli work, but not the gui. Wouldn't that lead to issues when the gui is used as rpc server via the bitcoin-cli?

So if you really want to go ahead with these changes, they need tests that fail on master but pass with your fixes for bitcoind as well as the bitcoin-qt and bitcoin-cli.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

Needs rebase
@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Closing "up for grabs".

@fanquake fanquake closed this Jan 5, 2019

@hebasto hebasto referenced this pull request Apr 21, 2019

Open

Fix datadir handling #15864

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.