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.
Don't forget to add a test.
421 | @@ -418,6 +422,20 @@ void ReleaseDirectoryLocks() 422 | dir_locks.clear(); 423 | } 424 | 425 | +bool DirIsWritable(const fs::path& directory) 426 | +{ 427 | + // Use a UUID for a unique temp filename. 428 | + fs::path tmpFile = directory / boost::lexical_cast<std::string>((boost::uuids::random_generator())());
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);
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 access(3), which just relied on <unistd.h>. I believe that MingW (which we use for Windows builds) provides a compatibility version of that header, so I think you can just use access() on the directory and completely remove the boost fs stuff.
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).
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.
and I'll note there aren't any credible proposed alternatives for boost::filesystem
Right - we'll just wait for std::filesystem from C++17. @theuni has a minimal patch somewhere that makes it work with that.
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 ", the second uses ', the third prints it unwrapped.
utACK 8674e74b47c1f6e86a367cfbc738fcc9812b616b