From fa0afe740843c308f6287b923f1f4d758cf2a3f6 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 14 Jul 2023 11:26:45 +0200 Subject: [PATCH] refactor: Return enum in LockDirectory This makes it easier to add more Error cases in the future. Also, add missing util namespace. --- src/init.cpp | 8 ++++--- src/test/util_tests.cpp | 50 +++++++++++++++++++++++------------------ src/util/fs_helpers.cpp | 13 ++++++----- src/util/fs_helpers.h | 8 ++++++- src/wallet/bdb.cpp | 2 +- 5 files changed, 48 insertions(+), 33 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 42331d37e8..9f86960ae5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1031,10 +1031,12 @@ static bool LockDataDirectory(bool probeOnly) if (!DirIsWritable(datadir)) { return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir))); } - if (!LockDirectory(datadir, ".lock", probeOnly)) { + switch (util::LockDirectory(datadir, ".lock", probeOnly)) { + 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)); - } - return true; + case util::LockResult::Success: return true; + } // no default case, so the compiler can warn about missing cases + assert(false); } bool AppInitSanityChecks(const kernel::Context& kernel) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 7d6c96ab40..3f79ad8553 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1106,15 +1106,15 @@ 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 + ResErrorLock, + ResUnlockSuccess, +}; [[noreturn]] static void TestOtherProcess(fs::path dirname, fs::path lockname, int fd) { @@ -1122,15 +1122,21 @@ 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::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; @@ -1167,51 +1173,51 @@ 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(LockDirectory(dirname, lockname), false); + BOOST_CHECK_EQUAL(util::LockDirectory(dirname, lockname), util::LockResult::ErrorLock); 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 @@ -1221,7 +1227,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); diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp index 2a9eb3502e..a5408dd0d7 100644 --- a/src/util/fs_helpers.cpp +++ b/src/util/fs_helpers.cpp @@ -53,15 +53,15 @@ static GlobalMutex cs_dir_locks; * is called. */ static std::map> dir_locks GUARDED_BY(cs_dir_locks); - -bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only) +namespace util { +LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only) { LOCK(cs_dir_locks); fs::path pathLockFile = directory / lockfile_name; // If a lock for this directory already exists in the map, don't try to re-lock it if (dir_locks.count(fs::PathToString(pathLockFile))) { - return true; + return LockResult::Success; } // Create empty lock file if it doesn't exist. @@ -69,15 +69,16 @@ bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, boo if (file) fclose(file); auto lock = std::make_unique(pathLockFile); if (!lock->TryLock()) { - return error("Error while attempting to lock directory %s: %s", fs::PathToString(directory), lock->GetReason()); + error("Error while attempting to lock directory %s: %s", fs::PathToString(directory), lock->GetReason()); + return LockResult::ErrorLock; } if (!probe_only) { // Lock successful and we're not just probing, put it into the map dir_locks.emplace(fs::PathToString(pathLockFile), std::move(lock)); } - return true; + return LockResult::Success; } - +} // namespace util void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name) { LOCK(cs_dir_locks); diff --git a/src/util/fs_helpers.h b/src/util/fs_helpers.h index e7db01a89b..3e30482004 100644 --- a/src/util/fs_helpers.h +++ b/src/util/fs_helpers.h @@ -35,7 +35,13 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length); */ [[nodiscard]] bool RenameOver(fs::path src, fs::path dest); -bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only = false); +namespace util { +enum class LockResult { + Success, + ErrorLock, +}; +[[nodiscard]] LockResult LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only = false); +} // namespace util void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name); bool DirIsWritable(const fs::path& directory); bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes = 0); diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 9ea43ca67c..cbf6c9b1ea 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -149,7 +149,7 @@ bool BerkeleyEnvironment::Open(bilingual_str& err) fs::path pathIn = fs::PathFromString(strPath); TryCreateDirectories(pathIn); - if (!LockDirectory(pathIn, ".walletlock")) { + if (util::LockDirectory(pathIn, ".walletlock") != util::LockResult::Success) { LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance may be using it.\n", strPath); err = strprintf(_("Error initializing wallet database environment %s!"), fs::quoted(fs::PathToString(Directory()))); return false;