Provide useful error message if datadir is not writable. #12630
Conversation
|
Don't forget to add a test. |
src/util.cpp
Outdated
| bool DirIsWritable(const fs::path& directory) | ||
| { | ||
| // Use a UUID for a unique temp filename. | ||
| fs::path tmpFile = directory / boost::lexical_cast<std::string>((boost::uuids::random_generator())()); |
There was a problem hiding this comment.
no need for lexical cast and uuid here: you can use fs::unique_path if it needs to be unique, but I'm not sure if hardcoding a name for the probe file wouldn't be better for troubleshooting.
Another (potentially better, as it will follow for wallet paths too) possibility would be to report a different failure from the LockDirectory function, as creating the lock file is done separately. E.g. change this to actually report an error:
// Create empty lock file if it doesn't exist.
FILE* file = fsbridge::fopen(pathLockFile, "a");
if (file) fclose(file);
There was a problem hiding this comment.
Agree, just improve error handling.
|
Concept ACK, but please try to solve this without introducing more Boost usage. We want to get rid of Boost – see https://github.com/bitcoin/bitcoin/projects/3 :-) |
|
Concept ACK. I agree with not needing UUID, etc., and not using the Boost libraries. |
|
On POSIX you can do this in one line of code with |
|
@eklitzke, access() works on Linux for me, but not Windows. I consistently get failure with errno ENOENT. I believe access() can be flaky with NFS as well, but I didn't test that myself. My most recent commit works on Windows and Linux. |
|
Alright in that case disregard. utACK 8a79879d2dfb5bcf1a020949044f246ce666e3d3 (and I'll note there aren't any credible proposed alternatives for boost::filesystem so I don't think that should be an objection). |
2218f89 to
8a79879
Compare
|
utACK after squash. I still think the check for write-ability could be rolled into LockDirectory, as that function creates the lock file by writing by definition (and that would take care of other directories too, such as wallet directories), but anyhow I'm ok with this solution too.
Right - we'll just wait for |
8a79879 to
8674e74
Compare
|
It works: $ src/bitcoind -datadir=/tmp/test1
Error: Specified data directory "/tmp/test1" does not exist.
$ mkdir /tmp/test2 && chmod 0 /tmp/test2
$ src/bitcoind -datadir=/tmp/test2
Error: Cannot write to data directory '/tmp/test2'; check permissions.
$ mkdir /tmp/test3
$ src/bitcoind -datadir=/tmp/test3 -daemon
$ src/bitcoind -datadir=/tmp/test3
Error: Cannot obtain a lock on data directory /tmp/test3. Bitcoin Core is probably already running.Nit: It's kind of funny that all three error messages wrap the path differently. The first puts it in |
|
utACK 8674e74 |
8674e74 Provide relevant error message if datadir is not writable. (murrayn) Pull request description: If the --datadir exists, but is not writable, the current error message on startup is 'Cannot obtain a lock on data directory foo. Bitcoin Core is probably already running.' This is misleading. I believe this PR addresses #11668, although the issue is not Windows-specific. Tree-SHA512: 10cbbaea433072aee4fb3e8938a72073c7a5c841f7a7685c9e12549c322b2925c7d34bac254ac33021b23132bfc352c058712bc9542298cf86f8fd9757f528b2
…ritable. 8674e74 Provide relevant error message if datadir is not writable. (murrayn) Pull request description: If the --datadir exists, but is not writable, the current error message on startup is 'Cannot obtain a lock on data directory foo. Bitcoin Core is probably already running.' This is misleading. I believe this PR addresses bitcoin#11668, although the issue is not Windows-specific. Tree-SHA512: 10cbbaea433072aee4fb3e8938a72073c7a5c841f7a7685c9e12549c322b2925c7d34bac254ac33021b23132bfc352c058712bc9542298cf86f8fd9757f528b2
…ritable. 8674e74 Provide relevant error message if datadir is not writable. (murrayn) Pull request description: If the --datadir exists, but is not writable, the current error message on startup is 'Cannot obtain a lock on data directory foo. Bitcoin Core is probably already running.' This is misleading. I believe this PR addresses bitcoin#11668, although the issue is not Windows-specific. Tree-SHA512: 10cbbaea433072aee4fb3e8938a72073c7a5c841f7a7685c9e12549c322b2925c7d34bac254ac33021b23132bfc352c058712bc9542298cf86f8fd9757f528b2
63e0be6 [Remove] By-pass logprint-scanner restriction. (furszy) 280ced3 utils: Fix broken Windows filelock (Chun Kuan Lee) be89860 utils: Convert Windows args to utf-8 string (Chun Kuan Lee) e8cfa6e Call unicode API on Windows (Chun Kuan Lee) 1a02a8a tests: Add test case for std::ios_base::ate (Chun Kuan Lee) 2e57cd4 Move boost/std fstream to fsbridge (furszy) 9d8bcd4 utils: Add fsbridge fstream function wrapper (Chun Kuan Lee) d59d48d utils: Convert fs error messages from multibyte to utf-8 (ken2812221) 9ef58cc Logging: use "fmterr" variable name for errors instead of general "e" that can be used by any other function. (furszy) dd94241 utils: Use _wfopen and _wreopen on Windows (Chun Kuan Lee) 3993641 add unicode compatible file_lock for Windows (Chun Kuan Lee) 48349f8 Provide relevant error message if datadir is not writable. (murrayn) Pull request description: As the software is currently using the ANSI encoding on Windows, the user's language settings could affect the proper functioning of the node/wallet, to the point of not be able to open some non-ASCII name files and directories. This solves the Windows encoding issues, completing the entire bitcoin#13869 work path (and some other required backports). Enabling for example users that use non-english characters in directories and file names to be accepted. Backported PRs: * bitcoin#12422. * bitcoin#12630. * bitcoin#13862. * bitcoin#13866. * bitcoin#13877. * bitcoin#13878. * bitcoin#13883. * bitcoin#13884. * bitcoin#13886. * bitcoin#13888. * bitcoin#14192. * bitcoin#13734. * bitcoin#14426. This is built on top of other two PRs that i have open #2423 and #2369. Solves old issues #940 and #2163. TODO: * Backport `assert_start_raises_init_error` and `ErrorMatch` in TestNode` (bitcoin#12718) ACKs for top commit: Fuzzbawls: ACK 63e0be6 random-zebra: ACK 63e0be6 and merging... Tree-SHA512: cb1f7c23abb5b7b3af50bba18652cc2cad93fd7c2fca9c16ffd3fee34c4c152a3b666dfa87fe6b44c430064dcdee4367144dcb4a41203c91b0173b805bdb3d7d
If the --datadir exists, but is not writable, the current error message on startup is 'Cannot obtain a lock on data directory foo. Bitcoin Core is probably already running.' This is misleading.
I believe this PR addresses #11668, although the issue is not Windows-specific.