From cabb2e5c24282c88ccc7fcaede854eaa8d7ff162 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 15 Jan 2025 18:03:43 +0000 Subject: [PATCH] refactor: introduce a more general LockDirectories for init No functional change. This is in preparation for adding additional directory locks on startup. --- src/bitcoind.cpp | 6 +++--- src/init.cpp | 31 ++++++++++++++++------------- src/init.h | 6 +++--- src/node/interfaces.cpp | 2 +- test/functional/feature_filelock.py | 2 +- 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index d210e2c8ba..ceb3c99410 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -228,10 +228,10 @@ static bool AppInit(NodeContext& node) return InitError(Untranslated("-daemon is not supported on this operating system")); #endif // HAVE_DECL_FORK } - // Lock data directory after daemonization - if (!AppInitLockDataDirectory()) + // Lock critical directories after daemonization + if (!AppInitLockDirectories()) { - // If locking the data directory failed, exit immediately + // If locking a directory failed, exit immediately return false; } fRet = AppInitInterfaces(node) && AppInitMain(node); diff --git a/src/init.cpp b/src/init.cpp index 9887c07165..13ef7ee13e 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1072,19 +1072,22 @@ bool AppInitParameterInteraction(const ArgsManager& args) return true; } -static bool LockDataDirectory(bool probeOnly) +static bool LockDirectory(const fs::path& dir, bool probeOnly) { - // Make sure only a single Bitcoin process is using the data directory. - const fs::path& datadir = gArgs.GetDataDirNet(); - switch (util::LockDirectory(datadir, ".lock", probeOnly)) { + // Make sure only a single process is using the directory. + switch (util::LockDirectory(dir, ".lock", probeOnly)) { case util::LockResult::ErrorWrite: - return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir))); + return InitError(strprintf(_("Cannot write to directory '%s'; check permissions."), fs::PathToString(dir))); case util::LockResult::ErrorLock: - return InitError(strprintf(_("Cannot obtain a lock on data directory %s. %s is probably already running."), fs::PathToString(datadir), CLIENT_NAME)); + return InitError(strprintf(_("Cannot obtain a lock on directory %s. %s is probably already running."), fs::PathToString(dir), CLIENT_NAME)); case util::LockResult::Success: return true; } // no default case, so the compiler can warn about missing cases assert(false); } +static bool LockDirectories(bool probeOnly) +{ + return LockDirectory(gArgs.GetDataDirNet(), probeOnly); +} bool AppInitSanityChecks(const kernel::Context& kernel) { @@ -1099,19 +1102,19 @@ bool AppInitSanityChecks(const kernel::Context& kernel) return InitError(strprintf(_("Elliptic curve cryptography sanity check failure. %s is shutting down."), CLIENT_NAME)); } - // Probe the data directory lock to give an early error message, if possible - // We cannot hold the data directory lock here, as the forking for daemon() hasn't yet happened, - // and a fork will cause weird behavior to it. - return LockDataDirectory(true); + // Probe the directory locks to give an early error message, if possible + // We cannot hold the directory locks here, as the forking for daemon() hasn't yet happened, + // and a fork will cause weird behavior to them. + return LockDirectories(true); } -bool AppInitLockDataDirectory() +bool AppInitLockDirectories() { - // After daemonization get the data directory lock again and hold on to it until exit + // After daemonization get the directory locks again and hold on to them until exit // This creates a slight window for a race condition to happen, however this condition is harmless: it // will at most make us exit without printing a message to console. - if (!LockDataDirectory(false)) { - // Detailed error printed inside LockDataDirectory + if (!LockDirectories(false)) { + // Detailed error printed inside LockDirectory return false; } return true; diff --git a/src/init.h b/src/init.h index 6d8a35d80e..6b60a4e147 100644 --- a/src/init.h +++ b/src/init.h @@ -55,11 +55,11 @@ bool AppInitParameterInteraction(const ArgsManager& args); */ bool AppInitSanityChecks(const kernel::Context& kernel); /** - * Lock bitcoin core data directory. + * Lock bitcoin core critical directories. * @note This should only be done after daemonization. Do not call Shutdown() if this function fails. * @pre Parameters should be parsed and config file should be read, AppInitSanityChecks should have been called. */ -bool AppInitLockDataDirectory(); +bool AppInitLockDirectories(); /** * Initialize node and wallet interface pointers. Has no prerequisites or side effects besides allocating memory. */ @@ -67,7 +67,7 @@ bool AppInitInterfaces(node::NodeContext& node); /** * Bitcoin core main initialization. * @note This should only be done after daemonization. Call Shutdown() if this function fails. - * @pre Parameters should be parsed and config file should be read, AppInitLockDataDirectory should have been called. + * @pre Parameters should be parsed and config file should be read, AppInitLockDirectories should have been called. */ bool AppInitMain(node::NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info = nullptr); diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f3b8c6a072..c084997cf7 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -116,7 +116,7 @@ public: m_context->ecc_context = std::make_unique(); if (!AppInitSanityChecks(*m_context->kernel)) return false; - if (!AppInitLockDataDirectory()) return false; + if (!AppInitLockDirectories()) return false; if (!AppInitInterfaces(*m_context)) return false; return true; diff --git a/test/functional/feature_filelock.py b/test/functional/feature_filelock.py index f56643c62e..7c05ca4f39 100755 --- a/test/functional/feature_filelock.py +++ b/test/functional/feature_filelock.py @@ -30,7 +30,7 @@ class FilelockTest(BitcoinTestFramework): self.log.info(f"Using datadir {datadir}") self.log.info("Check that we can't start a second bitcoind instance using the same datadir") - expected_msg = f"Error: Cannot obtain a lock on data directory {datadir}. {self.config['environment']['CLIENT_NAME']} is probably already running." + expected_msg = f"Error: Cannot obtain a lock on directory {datadir}. {self.config['environment']['CLIENT_NAME']} is probably already running." self.nodes[1].assert_start_raises_init_error(extra_args=[f'-datadir={self.nodes[0].datadir_path}', '-noserver'], expected_msg=expected_msg) self.log.info("Check that cookie and PID file are not deleted when attempting to start a second bitcoind using the same datadir")