Skip to content

util: if DataDir is a symbolic link, manually follow it #24267

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.

Sign up for GitHub

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

Jump to bottom
Closed
nymkappa wants to merge 1 commit into from
Closed

util: if DataDir is a symbolic link, manually follow it #24267

nymkappa wants to merge 1 commit into from

Conversation

@nymkappa
Copy link

@nymkappa nymkappa commented Feb 5, 2022 •
edited
Loading

Fixes #24257

$~/btc-things/bitcoin$ ln -s /media/DATA/Bitcoin /home/me/.bitcoin
$~/btc-things/bitcoin$ ./bitcoind 
************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: Not a directory [/home/me/.bitcoin]

Another related fix attempt can be found here #24266

@nymkappa nymkappa mentioned this pull request Feb 5, 2022
Merged
@nymkappa nymkappa marked this pull request as ready for review February 5, 2022 06:20
@nymkappa nymkappa marked this pull request as draft February 5, 2022 06:21
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 5, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24274 (Introduce GetFileArg and use it where possible by prusnak)
  • #24266 (util: Avoid buggy std::filesystem:::create_directories() call by hebasto)
  • #24265 (Drop StripRedundantLastElementsOfPath() function by hebasto)

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.

@DrahtBot DrahtBot mentioned this pull request Feb 5, 2022
Merged
hebasto
hebasto reviewed Feb 5, 2022
View reviewed changes
src/util/system.cpp
Comment on lines +449 to +452
if (!fs::is_directory(path)) {
path = "";
return path;
}
Copy link
Member

@hebasto hebasto Feb 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What scenarios these lines are required in?

Copy link
Author

@nymkappa nymkappa Feb 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A symbolic link may exist but point to a non existing location. For example, if your symbolic link point to an external hard drive, but it is not mounted.

Copy link
Author

@nymkappa nymkappa Feb 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example image

@nymkappa
Copy link
Author

nymkappa commented Feb 5, 2022 •
edited
Loading

I've tested the following scenarios. I run bitcoind from command line for each scenario and see what happens:

  • Create a valid symlink with ln -s /home/me/Documents .bitcoin - OK (it read and writes into /home/me/Documents properly)
  • Non existing ~/.bitcoin file/folder - OK (create a ~/.bitcoin folder)
  • Existing ~/.bitcoin folder - OK (load the content of ~/.bitcoin folder)
  • Create a broken symlink with ln -s /1234 .bitcoin - ??? (bitcoind does not start, but seems to crash, see below)
bitcoind: fs.cpp:39: fs::path fsbridge::AbsPathJoin(const fs::path&, const fs::path&): Assertion `base.is_absolute()' failed.
Aborted (core dumped)
  • Create a new wallet using bitcoin-cli - OK (create ~/.bitcoin/wallets/xxx folder)
  • Moved ~/.bitcoin/wallets/xxx to ~/.bitcoin/xxx - OK (I can load xxx wallet using bitcoin-cli)

@nymkappa nymkappa marked this pull request as ready for review February 5, 2022 12:26
@nymkappa nymkappa changed the title If DataDir is a symbolic link, manually follow it util::If DataDir is a symbolic link, manually follow it Feb 5, 2022
@nymkappa nymkappa changed the title util::If DataDir is a symbolic link, manually follow it util: if DataDir is a symbolic link, manually follow it Feb 5, 2022
@hebasto
Copy link
Member

hebasto commented Feb 5, 2022

@nymkappa

I've tested the following scenarios...

If there are behavior changes comparing to pre-20744 variant, could describe them in the PR description?

@DrahtBot DrahtBot mentioned this pull request Feb 5, 2022
Closed
@nymkappa
Copy link
Author

nymkappa commented Feb 6, 2022

@nymkappa

I've tested the following scenarios...

If there are behavior changes comparing to pre-20744 variant, could describe them in the PR description?

This is the only behavior change I've noticed from the previously mentioned scenario between master@04255073bbd2b8ea71ae8a9ff7433be499312758 (before #20744 was merged) and my pull request.

It looks like even though .bitcoin is a symbolic link (pointing to an invalid location), core does not try to follow it and instead tries to create a directory at that location.

  • Create a broken symlink with ln -s /1234 .bitcoin - ??? (bitcoind does not start, but seems to crash, see below)
************************
EXCEPTION: N5boost10filesystem16filesystem_errorE       
boost::filesystem::create_directory: File exists: "/home/me/.bitcoin"       
bitcoin in AppInit()       

bitcoind: chainparamsbase.cpp:35: const CBaseChainParams& BaseParams(): Assertion `globalChainBaseParams' failed.
Aborted (core dumped)

@hebasto
Copy link
Member

hebasto commented Feb 6, 2022

This is the only behavior change I've noticed from the previously mentioned scenario between master@04255073bbd2b8ea71ae8a9ff7433be499312758 (before #20744 was merged) and my pull request.

FWIW, #24266 preserves the pre-20744 behavior, and for a broken link bitcoind reports an exception with a meaningful message:

$ src/bitcoind


************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: File exists [/home/hebasto/.bitcoin/wallets]       
bitcoin in AppInit()

@nymkappa
Copy link
Author

nymkappa commented Feb 6, 2022

This is the only behavior change I've noticed from the previously mentioned scenario between master@04255073bbd2b8ea71ae8a9ff7433be499312758 (before #20744 was merged) and my pull request.

FWIW, #24266 preserves the pre-20744 behavior, and for a broken link bitcoind reports an exception with a meaningful message:

$ src/bitcoind


************************
EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
filesystem error: cannot create directories: File exists [/home/hebasto/.bitcoin/wallets]       
bitcoin in AppInit()

Closing this PR as #24266 provides a better fix. Thank you for your time hebasto 👍🏻

@nymkappa nymkappa closed this Feb 6, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto left review comments

Assignees

No one assigned

Labels

Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bitcoind and bitcoin-cli does not supports symbolic link anymore

3 participants

@nymkappa @DrahtBot @hebasto