Merge bitcoin/bitcoin#28075: util: Remove DirIsWritable, GetUniquePath

fa3da629a1aebcb4500803d7417feed8e34285b0 Remove DirIsWritable, GetUniquePath (MarcoFalke)
fad3a9793b71df5bb0b17cc3758cf3466d08c015 Return LockResult::ErrorWrite in LockDirectory (MarcoFalke)
fa0afe740843c308f6287b923f1f4d758cf2a3f6 refactor: Return enum in LockDirectory (MarcoFalke)

Pull request description:

  `GetUniquePath` is only used in tests and in `DirIsWritable`. The check by `DirIsWritable` is redundant with the check done in `LockDirectory`.

  Fix the redundancy by removing everything, except `LockDirectory`.

ACKs for top commit:
  TheCharlatan:
    Re-ACK fa3da629a1aebcb4500803d7417feed8e34285b0
  hebasto:
    ACK fa3da629a1aebcb4500803d7417feed8e34285b0, I have reviewed the code and it looks OK.

Tree-SHA512: e95f18cd586de7582e9c08ac7ddb860bfcfcbc8963804f45c5784c5e4c0598dc59ae7e45dd4daf30a5020dbf8433f5db2ad06e46a8676371982003790043c6c9

1 files changed, 36 insertions, 40 deletions

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 4d812544bd..47808a2a58 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -12,7 +12,6 @@
 #include <util/bitdeque.h>
 #include <util/fs.h>
 #include <util/fs_helpers.h>
-#include <util/getuniquepath.h>
 #include <util/message.h> // For MessageSign(), MessageVerify(), MESSAGE_MAGIC
 #include <util/moneystr.h>
 #include <util/overflow.h>
@@ -1106,15 +1105,16 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint)
     BOOST_CHECK(!ParseFixedPoint("31.999999999999999999999", 3, &amount));
 }
 
-static void TestOtherThread(fs::path dirname, fs::path lockname, bool *result)
-{
-    *result = LockDirectory(dirname, lockname);
-}
-
 #ifndef WIN32 // Cannot do this test on WIN32 due to lack of fork()
 static constexpr char LockCommand = 'L';
 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,
+};
 
 [[noreturn]] static void TestOtherProcess(fs::path dirname, fs::path lockname, int fd)
 {
@@ -1122,15 +1122,22 @@ static constexpr char ExitCommand = 'X';
     while (true) {
         int rv = read(fd, &ch, 1); // Wait for command
         assert(rv == 1);
-        switch(ch) {
+        switch (ch) {
         case LockCommand:
-            ch = LockDirectory(dirname, lockname);
+            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);
+            }();
             rv = write(fd, &ch, 1);
             assert(rv == 1);
             break;
         case UnlockCommand:
             ReleaseDirectoryLocks();
-            ch = true; // Always succeeds
+            ch = ResUnlockSuccess; // Always succeeds
             rv = write(fd, &ch, 1);
             assert(rv == 1);
             break;
@@ -1165,53 +1172,58 @@ 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(LockDirectory(dirname, lockname), false);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::ErrorWrite);
 
     fs::create_directories(dirname);
 
     // Probing lock on new directory should succeed
-    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success);
 
     // Persistent lock on new directory should succeed
-    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), true);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success);
 
     // Another lock on the directory from the same thread should succeed
-    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname), true);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::Success);
 
     // Another lock on the directory from a different thread within the same process should succeed
-    bool threadresult;
-    std::thread thr(TestOtherThread, dirname, lockname, &threadresult);
+    util::LockResult threadresult;
+    std::thread thr([&] { threadresult = util::LockDirectory(dirname, lockname); });
     thr.join();
-    BOOST_CHECK_EQUAL(threadresult, true);
+    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((bool)ch, false);
+    BOOST_CHECK_EQUAL(ch, ResErrorLock);
 
     // Give up our lock
     ReleaseDirectoryLocks();
     // Probing lock from our side now should succeed, but not hold on to the lock.
-    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success);
 
     // Try to acquire the lock in the child process, this should be successful.
     BOOST_CHECK_EQUAL(write(fd[1], &LockCommand, 1), 1);
     BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1);
-    BOOST_CHECK_EQUAL((bool)ch, true);
+    BOOST_CHECK_EQUAL(ch, ResSuccess);
 
     // When we try to probe the lock now, it should fail.
-    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), false);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::ErrorLock);
 
     // Unlock the lock in the child process
     BOOST_CHECK_EQUAL(write(fd[1], &UnlockCommand, 1), 1);
     BOOST_CHECK_EQUAL(read(fd[1], &ch, 1), 1);
-    BOOST_CHECK_EQUAL((bool)ch, true);
+    BOOST_CHECK_EQUAL(ch, ResUnlockSuccess);
 
     // When we try to probe the lock now, it should succeed.
-    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success);
 
     // Re-lock the lock in the child process, then wait for it to exit, check
     // successful return. After that, we check that exiting the process
@@ -1224,7 +1236,7 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
     BOOST_CHECK_EQUAL(write(fd[1], &ExitCommand, 1), 1);
     BOOST_CHECK_EQUAL(waitpid(pid, &processstatus, 0), pid);
     BOOST_CHECK_EQUAL(processstatus, 0);
-    BOOST_CHECK_EQUAL(LockDirectory(dirname, lockname, true), true);
+    BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname, true), util::LockResult::Success);
 
     // Restore SIGCHLD
     signal(SIGCHLD, old_handler);
@@ -1235,22 +1247,6 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
     fs::remove_all(dirname);
 }
 
-BOOST_AUTO_TEST_CASE(test_DirIsWritable)
-{
-    // Should be able to write to the data dir.
-    fs::path tmpdirname = m_args.GetDataDirBase();
-    BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), true);
-
-    // Should not be able to write to a non-existent dir.
-    tmpdirname = GetUniquePath(tmpdirname);
-    BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), false);
-
-    fs::create_directory(tmpdirname);
-    // Should be able to write to it now.
-    BOOST_CHECK_EQUAL(DirIsWritable(tmpdirname), true);
-    fs::remove(tmpdirname);
-}
-
 BOOST_AUTO_TEST_CASE(test_ToLower)
 {
     BOOST_CHECK_EQUAL(ToLower('@'), '@');