From b7f27cb0a07ede1ca8dafc5d4c7699d237a9f81b Mon Sep 17 00:00:00 2001 From: Martchus Date: Mon, 22 Feb 2021 23:44:06 +0100 Subject: [PATCH] Use locks to prevent multiple actions accessing the same db files and chroot dirs --- librepomgr/buildactions/buildactionprivate.h | 8 +++-- librepomgr/buildactions/conductbuild.cpp | 38 ++++++++++++-------- librepomgr/buildactions/reloaddatabase.cpp | 6 ++-- librepomgr/buildactions/repomanagement.cpp | 5 +++ librepomgr/serversetup.cpp | 10 ++++++ librepomgr/serversetup.h | 2 ++ 6 files changed, 49 insertions(+), 20 deletions(-) diff --git a/librepomgr/buildactions/buildactionprivate.h b/librepomgr/buildactions/buildactionprivate.h index fd58c13..6299b2c 100644 --- a/librepomgr/buildactions/buildactionprivate.h +++ b/librepomgr/buildactions/buildactionprivate.h @@ -138,7 +138,7 @@ public: template void launch(ChildArgs &&...childArgs); void registerWebSession(std::shared_ptr &&webSession); void registerNewDataHandler(std::function &&handler); - void assignLocks(AssociatedLocks &&locks = AssociatedLocks()); + AssociatedLocks &locks(); bool hasExited() const; private: @@ -215,9 +215,9 @@ inline BuildProcessSession::BuildProcessSession(BuildAction *buildAction, boost: { } -inline void BuildProcessSession::assignLocks(AssociatedLocks &&locks) +inline AssociatedLocks &BuildProcessSession::locks() { - m_locks = std::move(locks); + return m_locks; } inline bool BuildProcessSession::hasExited() const @@ -341,8 +341,10 @@ private: protected: std::string m_sourceRepoDirectory; std::string m_sourceDatabaseFile; + std::string m_sourceDatabaseLockName; std::string m_destinationRepoDirectory; std::string m_destinationDatabaseFile; + std::string m_destinationDatabaseLockName; std::string m_workingDirectory; std::vector m_fileNames; PackageMovementResult m_result; diff --git a/librepomgr/buildactions/conductbuild.cpp b/librepomgr/buildactions/conductbuild.cpp index b1f8d1b..b1b9044 100644 --- a/librepomgr/buildactions/conductbuild.cpp +++ b/librepomgr/buildactions/conductbuild.cpp @@ -901,19 +901,6 @@ InvocationResult ConductBuild::invokeMakechrootpkg( return InvocationResult::Error; } - // FIXME: lock the chroot directory to prevent other build tasks from using it (or does makechrootpkg already lock it?) - - // copy config files into chroot directory - try { - std::filesystem::copy_file(makepkgchrootSession->isStagingEnabled() ? m_pacmanStagingConfigPath : m_pacmanConfigPath, - buildRoot + "/etc/pacman.conf", std::filesystem::copy_options::overwrite_existing); - std::filesystem::copy_file(m_makepkgConfigPath, buildRoot + "/etc/makepkg.conf", std::filesystem::copy_options::overwrite_existing); - } catch (const std::filesystem::filesystem_error &e) { - auto writeLock = lockToWrite(lock); - packageProgress.error = "Unable to configure chroot \"" % buildRoot % "\": " + e.what(); - return InvocationResult::Error; - } - // determine options/variables to pass std::vector makechrootpkgFlags, makepkgFlags; // -> cleanup/upgrade @@ -936,16 +923,35 @@ InvocationResult ConductBuild::invokeMakechrootpkg( makepkgFlags.emplace_back("TEST_FILE_PATH=/testfiles"); } - // invoke makechrootpkg to build package - m_buildAction->log()(Phrases::InfoMessage, "Building ", packageName, '\n'); + // prepare process session auto processSession = m_buildAction->makeBuildProcess(packageName + " build", packageProgress.buildDirectory + "/build.log", std::bind(&ConductBuild::handleMakechrootpkgErrorsAndAddPackageToRepo, this, makepkgchrootSession, std::ref(packageName), std::ref(packageProgress), std::placeholders::_1, std::placeholders::_2)); processSession->registerNewDataHandler(BufferSearch("Updated version: ", "\e\n", "Starting build", std::bind(&ConductBuild::assignNewVersion, this, std::ref(packageName), std::ref(packageProgress), std::placeholders::_1))); + processSession->registerNewDataHandler(BufferSearch("Synchronizing chroot copy", "done", std::string_view(), + [processSession = processSession.get()](std::string &&) { processSession->locks().clear(); })); + + // lock the chroot directory to prevent other build tasks from using it + m_buildAction->log()(Phrases::InfoMessage, "Building ", packageName, '\n'); + auto chrootLock = m_setup.locks.acquireToWrite(buildRoot); + + // copy config files into chroot directory + try { + std::filesystem::copy_file(makepkgchrootSession->isStagingEnabled() ? m_pacmanStagingConfigPath : m_pacmanConfigPath, + buildRoot + "/etc/pacman.conf", std::filesystem::copy_options::overwrite_existing); + std::filesystem::copy_file(m_makepkgConfigPath, buildRoot + "/etc/makepkg.conf", std::filesystem::copy_options::overwrite_existing); + } catch (const std::filesystem::filesystem_error &e) { + auto writeLock = lockToWrite(lock); + packageProgress.error = "Unable to configure chroot \"" % buildRoot % "\": " + e.what(); + return InvocationResult::Error; + } + + // invoke makechrootpkg to build package m_buildAction->log()(Phrases::InfoMessage, "Invoking makechrootpkg for ", packageName, " via ", m_makeChrootPkgPath.string(), '\n', ps(Phrases::SubMessage), "build dir: ", packageProgress.buildDirectory, '\n', ps(Phrases::SubMessage), "chroot dir: ", chrootDir, '\n', ps(Phrases::SubMessage), "chroot user: ", packageProgress.chrootUser, '\n'); + processSession->locks().emplace_back(std::move(chrootLock)); processSession->launch(boost::process::start_dir(packageProgress.buildDirectory), m_makeChrootPkgPath, makechrootpkgFlags, "-C", m_globalPackageCacheDir, "-r", chrootDir, "-l", packageProgress.chrootUser, packageProgress.makechrootpkgFlags, "--", makepkgFlags, packageProgress.makepkgFlags); @@ -1111,6 +1117,8 @@ void ConductBuild::addPackageToRepo( auto processSession = m_buildAction->makeBuildProcess("repo-add for " + packageName, packageProgress.buildDirectory + "/repo-add.log", std::bind(&ConductBuild::handleRepoAddErrorsAndMakeNextPackage, this, makepkgchrootSession, std::ref(packageName), std::ref(packageProgress), std::placeholders::_1, std::placeholders::_2)); + processSession->locks().emplace_back(m_setup.locks.acquireToWrite( + ServiceSetup::Locks::forDatabase(needsStaging ? m_buildPreparation.targetDb : m_buildPreparation.stagingDb, m_buildPreparation.targetArch))); processSession->launch(boost::process::start_dir(repoPath), m_repoAddPath, dbFilePath, binaryPackageNames); m_buildAction->log()(Phrases::InfoMessage, "Adding ", packageName, " to repo\n", ps(Phrases::SubMessage), "repo path: ", repoPath, '\n', ps(Phrases::SubMessage), "db path: ", dbFilePath, '\n', ps(Phrases::SubMessage), "package(s): ", joinStrings(binaryPackageNames), '\n'); diff --git a/librepomgr/buildactions/reloaddatabase.cpp b/librepomgr/buildactions/reloaddatabase.cpp index aad3f84..4c6b952 100644 --- a/librepomgr/buildactions/reloaddatabase.cpp +++ b/librepomgr/buildactions/reloaddatabase.cpp @@ -74,11 +74,13 @@ void ReloadDatabase::run() m_buildAction->appendOutput( Phrases::InfoMessage, "Loading database \"", dbName, '@', dbArch, "\" from local file \"", dbPath, "\"\n"); try { + auto dbFileLock = m_setup.locks.acquireToRead({ ServiceSetup::Locks::forDatabase(dbName, dbArch) }); const auto lastModified = LibPkg::lastModified(dbPath); auto dbFile = LibPkg::extractFiles(dbPath, &LibPkg::Database::isFileRelevant); auto packages = LibPkg::Package::fromDatabaseFile(move(dbFile)); - auto lock = m_setup.config.lockToWrite(); - auto db = m_setup.config.findDatabase(dbName, dbArch); + dbFileLock.unlock(); + const auto configLock = m_setup.config.lockToWrite(); + auto *const db = m_setup.config.findDatabase(dbName, dbArch); if (!db) { m_buildAction->appendOutput( Phrases::ErrorMessage, "Loaded database file for \"", dbName, '@', dbArch, "\" but it no longer exists; discarding\n"); diff --git a/librepomgr/buildactions/repomanagement.cpp b/librepomgr/buildactions/repomanagement.cpp index 739136e..cafb346 100644 --- a/librepomgr/buildactions/repomanagement.cpp +++ b/librepomgr/buildactions/repomanagement.cpp @@ -55,10 +55,12 @@ bool PackageMovementAction::prepareRepoAction(RequiredDatabases requiredDatabase const auto *const destinationDb = *m_destinationDbs.begin(); m_destinationRepoDirectory = destinationDb->localPkgDir; m_destinationDatabaseFile = fileName(destinationDb->path); + m_destinationDatabaseLockName = ServiceSetup::Locks::forDatabase(*destinationDb); if (requiredDatabases & RequiredDatabases::OneSource) { const auto *const sourceDb = *m_sourceDbs.begin(); m_sourceRepoDirectory = sourceDb->localPkgDir; m_sourceDatabaseFile = fileName(sourceDb->path); + m_sourceDatabaseLockName = ServiceSetup::Locks::forDatabase(*sourceDb); } locatePackages(); configReadLock = std::monostate(); @@ -154,6 +156,7 @@ void RemovePackages::run() // remove package from database file auto repoRemoveProcess = m_buildAction->makeBuildProcess("repo-remove", m_workingDirectory + "/repo-remove.log", std::bind(&RemovePackages::handleRepoRemoveResult, this, std::placeholders::_1, std::placeholders::_2)); + repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_destinationDatabaseLockName)); repoRemoveProcess->launch( boost::process::start_dir(m_destinationRepoDirectory), m_repoRemovePath, m_destinationDatabaseFile, m_result.processedPackages); m_buildAction->log()(Phrases::InfoMessage, "Invoking repo-remove within \"", m_destinationRepoDirectory, "\" for \"", m_destinationDatabaseFile, @@ -270,11 +273,13 @@ void MovePackages::run() // add packages to database file of destination repo auto repoAddProcess = m_buildAction->makeBuildProcess("repo-add", m_workingDirectory + "/repo-add.log", std::bind(&MovePackages::handleRepoAddResult, this, processSession, std::placeholders::_1, std::placeholders::_2)); + repoAddProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_destinationDatabaseLockName)); repoAddProcess->launch(boost::process::start_dir(m_destinationRepoDirectory), m_repoAddPath, m_destinationDatabaseFile, m_fileNames); // remove package from database file of source repo auto repoRemoveProcess = m_buildAction->makeBuildProcess("repo-remove", m_workingDirectory + "/repo-remove.log", std::bind(&MovePackages::handleRepoRemoveResult, this, processSession, std::placeholders::_1, std::placeholders::_2)); + repoRemoveProcess->locks().emplace_back(m_setup.locks.acquireToWrite(m_sourceDatabaseLockName)); repoRemoveProcess->launch(boost::process::start_dir(m_sourceRepoDirectory), m_repoRemovePath, m_sourceDatabaseFile, m_result.processedPackages); m_buildAction->log()(ps(Phrases::InfoMessage), "Invoking repo-add within \"", m_destinationRepoDirectory, "\" for \"", m_destinationDatabaseFile, diff --git a/librepomgr/serversetup.cpp b/librepomgr/serversetup.cpp index 98f0d1f..26df2a0 100644 --- a/librepomgr/serversetup.cpp +++ b/librepomgr/serversetup.cpp @@ -663,6 +663,16 @@ void ServiceSetup::Locks::clear() } } +std::string ServiceSetup::Locks::forDatabase(std::string_view dbName, std::string_view dbArch) +{ + return dbName % '@' + dbArch; +} + +std::string ServiceSetup::Locks::forDatabase(const LibPkg::Database &db) +{ + return forDatabase(db.name, db.arch); +} + ServiceStatus::ServiceStatus(const ServiceSetup &setup) : version(applicationInfo.version) , config(setup.config.computeStatus()) diff --git a/librepomgr/serversetup.h b/librepomgr/serversetup.h index c6d7b23..426de05 100644 --- a/librepomgr/serversetup.h +++ b/librepomgr/serversetup.h @@ -125,6 +125,8 @@ struct LIBREPOMGR_EXPORT ServiceSetup : public LibPkg::Lockable { [[nodiscard]] std::unique_lock acquireToWrite(const std::string &lockName); [[nodiscard]] std::unique_lock acquireToWrite(std::shared_lock &readLock, const std::string &lockName); void clear(); + static std::string forDatabase(std::string_view dbName, std::string_view dbArch); + static std::string forDatabase(const LibPkg::Database &db); private: std::mutex m_mutex;