Return LockResult::ErrorWrite in LockDirectory

This allows the caller to remove a call to DirIsWritable(), which did a
similar check. Users should not notice any different behavior.

Author: MarcoFalke

src/init.cpp

@@ -1028,10 +1028,9 @@ static bool LockDataDirectory(bool probeOnly)
 {
     // Make sure only a single Bitcoin process is using the data directory.
     const fs::path& datadir = gArgs.GetDataDirNet();
-    if (!DirIsWritable(datadir)) {
-        return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir)));
-    }
     switch (util::LockDirectory(datadir, ".lock", probeOnly)) {
+    case util::LockResult::ErrorWrite:
+        return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir)));
     case util::LockResult::ErrorLock:
         return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), fs::PathToString(datadir), PACKAGE_NAME));
     case util::LockResult::Success: return true;

src/test/util_tests.cpp

@@ -1112,6 +1112,7 @@ static constexpr char UnlockCommand = 'U';
 static constexpr char ExitCommand = 'X';
 enum : char {
     ResSuccess = 2, // Start with 2 to avoid accidental collision with common values 0 and 1
+    ResErrorWrite,
     ResErrorLock,
     ResUnlockSuccess,
 };
@@ -1127,6 +1128,7 @@ enum : char {
             ch = [&] {
                 switch (util::LockDirectory(dirname, lockname)) {
                 case util::LockResult::Success: return ResSuccess;
+                case util::LockResult::ErrorWrite: return ResErrorWrite;
                 case util::LockResult::ErrorLock: return ResErrorLock;
                 } // no default case, so the compiler can warn about missing cases
                 assert(false);
@@ -1171,9 +1173,15 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
         TestOtherProcess(dirname, lockname, fd[0]);
     }
     BOOST_CHECK_EQUAL(close(fd[0]), 0); // Parent: close child end
+
+    char ch;
+    // Lock on non-existent directory should fail
+    BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1);
+    BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1);
+    BOOST_CHECK_EQUAL(ch, ResErrorWrite);
 #endif
     // Lock on non-existent directory should fail
-    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::ErrorLock);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::ErrorWrite);
 
     fs::create_directories(dirname);
 
@@ -1193,7 +1201,6 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
     BOOST_CHECK_EQUAL(threadresult, util::LockResult::Success);
 #ifndef WIN32
     // Try to acquire lock in child process while we're holding it, this should fail.
-    char ch;
     BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1);
     BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1);
     BOOST_CHECK_EQUAL(ch, ResErrorLock);

src/util/fs_helpers.cpp

@@ -65,8 +65,11 @@ LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_nam
     }
 
     // Create empty lock file if it doesn't exist.
-    FILE* file = fsbridge::fopen(pathLockFile, "a");
-    if (file) fclose(file);
+    if (auto created{fsbridge::fopen(pathLockFile, "a")}) {
+        std::fclose(created);
+    } else {
+        return LockResult::ErrorWrite;
+    }
     auto lock = std::make_unique<fsbridge::FileLock>(pathLockFile);
     if (!lock->TryLock()) {
         error("Error while attempting to lock directory %s: %s", fs::PathToString(directory), lock->GetReason());

src/util/fs_helpers.h

@@ -38,6 +38,7 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length);
 namespace util {
 enum class LockResult {
     Success,
+    ErrorWrite,
     ErrorLock,
 };
 [[nodiscard]] LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only = false);