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

nymkappa commented at 3:15 am on February 5, 2022: none

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

Another related fix attempt can be found here https://github.com/bitcoin/bitcoin/pull/24266

If DataDir is a symbolic link, manually follow it 247a5c17c1

DrahtBot added the label Utils/log/libs on Feb 5, 2022

nymkappa marked this as ready for review on Feb 5, 2022

nymkappa marked this as a draft on Feb 5, 2022

DrahtBot commented at 8:58 am on February 5, 2022: member

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.

in src/util/system.cpp:452 in 247a5c17c1

447+    if (fs::is_symlink(path)) {
448+        path = fs::read_symlink(path);
449+        if (!fs::is_directory(path)) {
450+            path = "";
451+            return path;
452+        }

hebasto commented at 9:07 am on February 5, 2022:

What scenarios these lines are required in?

nymkappa commented at 9:08 am on February 5, 2022:

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.

nymkappa commented at 9:10 am on February 5, 2022:

For example

nymkappa commented at 12:18 pm on February 5, 2022: none

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)

0bitcoind: fs.cpp:39: fs::path fsbridge::AbsPathJoin(const fs::path&, const fs::path&): Assertion `base.is_absolute()' failed.
1Aborted (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 marked this as ready for review on Feb 5, 2022

nymkappa renamed this:
~~If DataDir is a symbolic link, manually follow it~~
util::If DataDir is a symbolic link, manually follow it
on Feb 5, 2022

nymkappa renamed this:
~~util::If DataDir is a symbolic link, manually follow it~~
util: if `DataDir` is a symbolic link, manually follow it
on Feb 5, 2022

hebasto commented at 4:47 pm on February 5, 2022: member

@nymkappa

I’ve tested the following scenarios…

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

nymkappa commented at 2:29 am on February 6, 2022: none

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

0************************
1EXCEPTION: N5boost10filesystem16filesystem_errorE       
2boost::filesystem::create_directory: File exists: "/home/me/.bitcoin"       
3bitcoin in AppInit()       
4
5bitcoind: chainparamsbase.cpp:35: const CBaseChainParams& BaseParams(): Assertion `globalChainBaseParams' failed.
6Aborted (core dumped)

hebasto commented at 7:20 am on February 6, 2022: member

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:

0$ src/bitcoind
1
2
3************************
4EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
5filesystem error: cannot create directories: File exists [/home/hebasto/.bitcoin/wallets]       
6bitcoin in AppInit()

nymkappa commented at 8:10 am on February 6, 2022: none

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:
0$ src/bitcoind
1
2
3************************
4EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE       
5filesystem error: cannot create directories: File exists [/home/hebasto/.bitcoin/wallets]       
6bitcoin in AppInit()       

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

nymkappa closed this on Feb 6, 2022

DrahtBot locked this on Feb 6, 2023

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

Conflicts

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