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.
bitcoin · Jul 20, 2023 · fa5baee · fa5baee
1 parent dddd818
commit fa5baee
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 6 deletions.
diff --git a/src/init.cpp b/src/init.cpp
@@ -1012,10 +1012,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;

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
@@ -1122,6 +1122,7 @@ static constexpr char ExitCommand = 'X';
             ch = [&] {
                 switch (util::LockDirectory(dirname, lockname)) {
                 case util::LockResult::Success: return 1;
+                case util::LockResult::ErrorWrite: return 2;
                 case util::LockResult::ErrorLock: return 0;
                 } // no default case, so the compiler can warn about missing cases
                 assert(false);
@@ -1168,7 +1169,7 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
     BOOST_CHECK_EQUAL(close(fd[0]), 0); // Parent: close child end
 #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);
 

diff --git a/src/util/fs_helpers.cpp b/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());

diff --git a/src/util/fs_helpers.h b/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);