Lock from the time system repository is loaded until exit

VERSION.cmake

@@ -1,5 +1,5 @@
 set(DEFAULT_PROJECT_VERSION_PRIME 5)
-set(DEFAULT_PROJECT_VERSION_MAJOR 3)
+set(DEFAULT_PROJECT_VERSION_MAJOR 4)
 set(DEFAULT_PROJECT_VERSION_MINOR 0)
 set(DEFAULT_PROJECT_VERSION_MICRO 0)
 

bindings/libdnf5/utils.i

@@ -17,6 +17,7 @@
 
     #include "libdnf5/utils/locale.hpp"
     #include "libdnf5/utils/patterns.hpp"
+    #include "libdnf5/utils/locker.hpp"
 %}
 
 #define CV __perl_CV
@@ -35,3 +36,5 @@
 
 // Deletes any previously defined catches
 %catches();
+
+%include "libdnf5/utils/locker.hpp"

dnf5.spec

@@ -1,5 +1,5 @@
 %global project_version_prime 5
-%global project_version_major 3
+%global project_version_major 4
 %global project_version_minor 0
 %global project_version_micro 0
 
@@ -430,6 +430,7 @@ Package management library.
 %verify(not md5 size mtime) %attr(0644, root, root) %ghost %{_prefix}/lib/sysimage/libdnf5/packages.toml
 %verify(not md5 size mtime) %attr(0644, root, root) %ghost %{_prefix}/lib/sysimage/libdnf5/system.toml
 %verify(not md5 size mtime) %attr(0644, root, root) %ghost %{_prefix}/lib/sysimage/libdnf5/transaction_history.sqlite{,-shm,-wal}
+%verify(not md5 size mtime) %attr(0664, root, root) %ghost %{_prefix}/lib/sysimage/libdnf5/system-repo.lock
 %license lgpl-2.1.txt
 %ghost %attr(0755, root, root) %dir %{_var}/cache/libdnf5
 %ghost %attr(0755, root, root) %dir %{_sharedstatedir}/dnf
@@ -1029,7 +1030,8 @@ for file in \
     environments.toml groups.toml modules.toml nevras.toml packages.toml \
     system.toml \
     transaction_history.sqlite transaction_history.sqlite-shm \
-    transaction_history.sqlite-wal
+    transaction_history.sqlite-wal \
+    system-repo.lock
 do
     touch %{buildroot}%{_prefix}/lib/sysimage/libdnf5/$file
 done

dnf5/commands/makecache/makecache.cpp

@@ -57,7 +57,7 @@ void MakeCacheCommand::run() {
         return;
     }
 
-    ctx.load_repos(false);
+    ctx.load_repos(false, true);
 
     std::cout << "Metadata cache created." << std::endl;
 }

dnf5/context.cpp

@@ -32,6 +32,7 @@
 #include <libdnf5-cli/utils/userconfirm.hpp>
 #include <libdnf5/base/base.hpp>
 #include <libdnf5/base/goal.hpp>
+#include <libdnf5/common/proc.hpp>
 #include <libdnf5/conf/const.hpp>
 #include <libdnf5/rpm/package_query.hpp>
 #include <libdnf5/rpm/package_set.hpp>
@@ -145,7 +146,7 @@ class Context::Impl {
         const std::vector<std::string> & bzs,
         const std::vector<std::string> & cves);
 
-    void load_repos(bool load_system);
+    void load_repos(bool load_system, bool load_available);
 
     void store_offline(libdnf5::base::Transaction & transaction);
 
@@ -194,6 +195,9 @@ class Context::Impl {
     void set_load_system_repo(bool on) { load_system_repo = on; }
     bool get_load_system_repo() const noexcept { return load_system_repo; }
 
+    void set_will_modify_system(bool modify) { will_modify_system = modify; }
+    bool get_will_modify_system() const noexcept { return will_modify_system; }
+
     void set_load_available_repos(LoadAvailableRepos which) { load_available_repos = which; }
     LoadAvailableRepos get_load_available_repos() const noexcept { return load_available_repos; }
 
@@ -276,6 +280,7 @@ class Context::Impl {
 
     bool load_system_repo{false};
     LoadAvailableRepos load_available_repos{LoadAvailableRepos::NONE};
+    bool will_modify_system{true};
     bool show_version{false};
 };
 
@@ -349,26 +354,74 @@ void Context::Impl::update_repo_metadata_from_advisory_options(
     }
 }
 
-void Context::Impl::load_repos(bool load_system) {
-    libdnf5::repo::RepoQuery repos(base);
-    repos.filter_enabled(true);
-    repos.filter_type(libdnf5::repo::Repo::Type::SYSTEM, libdnf5::sack::QueryCmp::NEQ);
+void Context::Impl::load_repos(bool load_system, bool load_available) {
+    if (load_available) {
+        libdnf5::repo::RepoQuery repos(base);
+        repos.filter_enabled(true);
+        repos.filter_type(libdnf5::repo::Repo::Type::SYSTEM, libdnf5::sack::QueryCmp::NEQ);
 
-    for (auto & repo : repos) {
-        repo->set_callbacks(std::make_unique<dnf5::KeyImportRepoCB>(base.get_config()));
+        for (auto & repo : repos) {
+            repo->set_callbacks(std::make_unique<dnf5::KeyImportRepoCB>(base.get_config()));
+        }
     }
 
-    print_info(_("Updating and loading repositories:"));
-    if (load_system) {
+    const auto skip_system_repo_lock = base.get_config().get_skip_system_repo_lock_option().get_value();
+    if (load_system && !skip_system_repo_lock) {
+        const auto lock_access_type =
+            get_will_modify_system() ? libdnf5::utils::LockAccess::WRITE : libdnf5::utils::LockAccess::READ;
+        if (!base.lock_system_repo(lock_access_type, libdnf5::utils::LockBlocking::NON_BLOCKING)) {
+            // A lock on the system repo could not immediately be acquired.
+            // Gather and display information about other processes in line for the lock, then wait.
+            const auto & lock_file_path = base.get_system_repo_lock()->get_path();
+            std::set<pid_t> pids;
+            try {
+                pids = libdnf5::fuser(lock_file_path);
+            } catch (const libdnf5::SystemError &) {
+                // The lock could have been released immediately after lock_system_repo fails,
+                // or we might not have permission to read procfs.
+            }
+            pids.erase(getpid());
+            if (pids.empty()) {
+                print_info(
+                    _("Waiting for a lock on the system repository; another process is currently accessing it. Use the "
+                      "\"--skip-file-locks\" option to bypass the lock."));
+            } else {
+                print_info(
+                    _("Waiting for a lock on the system repository. The following processes are currently accessing "
+                      "it:"));
+                for (const auto & pid : pids) {
+                    try {
+                        const auto & cmdline = libdnf5::pid_cmdline(pid);
+                        print_info(libdnf5::utils::sformat("{}\t{}", pid, libdnf5::utils::string::join(cmdline, " ")));
+                    } catch (const libdnf5::SystemError &) {
+                        print_info(libdnf5::utils::sformat("{}", pid));
+                    }
+                }
+                print_info(_("Use the \"--skip-file-locks\" option to bypass the lock."));
+            }
+            base.lock_system_repo(lock_access_type, libdnf5::utils::LockBlocking::BLOCKING);
+        }
+    }
+
+    if (load_available) {
+        // If we're only loading the system repo, we can skip the message.
+        print_info(_("Updating and loading repositories:"));
+    }
+
+    if (load_system && load_available) {
         base.get_repo_sack()->load_repos();
-    } else {
+    } else if (load_system) {
+        base.get_repo_sack()->load_repos(libdnf5::repo::Repo::Type::SYSTEM);
+    } else if (load_available) {
         base.get_repo_sack()->load_repos(libdnf5::repo::Repo::Type::AVAILABLE);
     }
 
     if (auto download_callbacks = dynamic_cast<DownloadCallbacks *>(base.get_download_callbacks())) {
         download_callbacks->reset_progress_bar();
     }
-    print_info(_("Repositories loaded."));
+    if (load_available) {
+        print_info(_("Repositories loaded."));
+    }
 }
 
 void Context::Impl::store_offline(libdnf5::base::Transaction & transaction) {
@@ -601,8 +654,8 @@ void Context::update_repo_metadata_from_advisory_options(
         names, security, bugfix, enhancement, newpackage, severity, bzs, cves);
 }
 
-void Context::load_repos(bool load_system) {
-    p_impl->load_repos(load_system);
+void Context::load_repos(bool load_system, bool load_available) {
+    p_impl->load_repos(load_system, load_available);
 }
 
 void Context::store_offline(libdnf5::base::Transaction & transaction) {
@@ -692,6 +745,13 @@ bool Context::get_load_system_repo() const noexcept {
     return p_impl->get_load_system_repo();
 }
 
+void Context::set_will_modify_system(bool modify) {
+    p_impl->set_will_modify_system(modify);
+}
+bool Context::get_will_modify_system() const noexcept {
+    return p_impl->get_will_modify_system();
+}
+
 void Context::set_load_available_repos(LoadAvailableRepos which) {
     p_impl->set_load_available_repos(which);
 }
@@ -1171,13 +1231,13 @@ std::vector<std::string> match_specs(
                 repo->get_config().get_skip_if_unavailable_option().set(libdnf5::Option::Priority::RUNTIME, true);
             }
 
-            ctx.load_repos(installed);
+            ctx.load_repos(installed, true);
         } catch (...) {
             // Ignores errors when completing available packages, other completions may still work.
         }
     } else if (installed) {
         try {
-            base.get_repo_sack()->load_repos(libdnf5::repo::Repo::Type::SYSTEM);
+            ctx.load_repos(true, false);
         } catch (...) {
             // Ignores errors when completing installed packages, other completions may still work.
         }

dnf5/include/dnf5/context.hpp

@@ -77,7 +77,7 @@ class DNF_API Context : public libdnf5::cli::session::Session {
         const std::vector<std::string> & cves);
 
     /// Sets callbacks for repositories and loads them, updating metadata if necessary.
-    void load_repos(bool load_system);
+    void load_repos(bool load_system, bool load_available);
 
     void store_offline(libdnf5::base::Transaction & transaction);
 
@@ -133,6 +133,9 @@ class DNF_API Context : public libdnf5::cli::session::Session {
     void set_load_system_repo(bool on);
     bool get_load_system_repo() const noexcept;
 
+    void set_will_modify_system(bool will_modify_system);
+    bool get_will_modify_system() const noexcept;
+
     void set_load_available_repos(LoadAvailableRepos which);
     LoadAvailableRepos get_load_available_repos() const noexcept;
 

dnf5/main.cpp

@@ -668,6 +668,15 @@ void RootCommand::set_argument_parser() {
         global_options_group->register_argument(forcearch);
     }
 
+    auto skip_file_locks = parser.add_new_named_arg("skip-file-locks");
+    skip_file_locks->set_long_name("skip-file-locks");
+    skip_file_locks->set_description(_("Skip acquiring file locks, such as the lock on the system repository"));
+    skip_file_locks->set_const_value("true");
+    // For now, --skip-file-locks applies to just the system repository lock.
+    // It should be updated to ignore metadata locks, etc. when we implement those.
+    skip_file_locks->link_value(&config.get_skip_system_repo_lock_option());
+    global_options_group->register_argument(skip_file_locks);
+
     register_group_with_args(cmd, *global_options_group);
 
     parser.set_inherit_named_args(true);
@@ -1242,19 +1251,29 @@ static bool cmd_requires_privileges(dnf5::Context & context) {
 }
 
 static bool user_has_privileges(dnf5::Context & context) {
-    std::filesystem::path lock_file_path = context.get_base().get_config().get_installroot_option().get_value();
-    lock_file_path /= std::filesystem::path(libdnf5::TRANSACTION_LOCK_FILEPATH).relative_path();
-    lock_file_path += ".tmp";
+    const auto & installroot = context.get_base().get_config().get_installroot_option().get_value();
+
+    const auto & transaction_lock_path =
+        installroot / std::filesystem::path{libdnf5::TRANSACTION_LOCK_FILEPATH}.relative_path();
+
+    const auto & system_state_dir = context.get_base().get_config().get_system_state_dir_option().get_value();
+    const auto & system_repo_lock_path =
+        installroot / std::filesystem::path{system_state_dir}.relative_path() / libdnf5::SYSTEM_REPO_LOCK_FILENAME;
 
     try {
-        std::filesystem::create_directories(lock_file_path.parent_path());
-        libdnf5::utils::Locker locker(lock_file_path);
-        return locker.write_lock();
+        std::filesystem::create_directories(transaction_lock_path.parent_path());
+        libdnf5::utils::Locker transaction_locker(transaction_lock_path, false);
+        transaction_locker.open_file(libdnf5::utils::LockAccess::WRITE);
+
+        std::filesystem::create_directories(system_repo_lock_path.parent_path());
+        libdnf5::utils::Locker system_repo_locker(system_repo_lock_path, true);
+        system_repo_locker.open_file(libdnf5::utils::LockAccess::WRITE);
     } catch (libdnf5::SystemError & ex) {
         return false;
     } catch (std::filesystem::filesystem_error & ex) {
         return false;
     }
+    return true;
 }
 
 int main(int argc, char * argv[]) try {
@@ -1517,20 +1536,18 @@ int main(int argc, char * argv[]) try {
                 dump_repository_configuration(context, repo_id_list);
             }
 
+            const bool will_modify_system{cmd_requires_privileges(context)};
             if (cmd_requires_privileges(context) && !user_has_privileges(context)) {
                 throw libdnf5::cli::InsufficientPrivilegesError(
                     M_("The requested operation requires superuser privileges. Please log in as a user with elevated "
                        "rights, or use the \"--assumeno\" or \"--downloadonly\" options to run the command without "
                        "modifying the system state."));
             }
 
-            {
-                if (context.get_load_available_repos() != dnf5::Context::LoadAvailableRepos::NONE) {
-                    context.load_repos(context.get_load_system_repo());
-                } else if (context.get_load_system_repo()) {
-                    repo_sack->load_repos(libdnf5::repo::Repo::Type::SYSTEM);
-                }
-            }
+            context.set_will_modify_system(will_modify_system);
+
+            const auto load_available = context.get_load_available_repos() != dnf5::Context::LoadAvailableRepos::NONE;
+            context.load_repos(context.get_load_system_repo(), load_available);
 
             command->load_additional_packages();
 

doc/dnf5.8.rst

@@ -323,6 +323,10 @@ Following options are applicable in the general context for any ``dnf5`` command
 ``--setvar=VAR_NAME=VALUE``
     | Override a ``DNF5`` variable value, like ``arch``, ``releasever``, etc.
 
+``--skip-file-locks``
+    | Skip acquiring file locks, such as the lock on the system repository.
+    | See :ref:`skip_sytem_repo_lock <skip_system_repo_lock_options-label>` for more info.
+
 ``--show-new-leaves``
     | Show newly installed leaf packages and packages that became leaves after a transaction.
 

doc/dnf5.conf.5.rst

@@ -523,6 +523,17 @@ repository configuration file should aside from repo ID consists of baseurl, met
 
     Default: ``False``.
 
+.. _skip_system_repo_lock_options-label:
+
+``skip_system_repo_lock``
+    :ref:`boolean <boolean-label>`
+
+    Skip acquiring a lock on the system repository (equivalent to the RPM
+    database). The lock is used to prevent processes from reading the system
+    repository while another process is running a transaction. Unprivileged
+    users are allowed to acquire a read lock on the system repository, so
+    ``skip_system_repo_lock=true`` may be used to ignore their lock.
+
 .. _skip_unavailable_options-label:
 
 ``skip_unavailable``

include/libdnf5/base/base.hpp

@@ -37,6 +37,8 @@
 #include "libdnf5/rpm/package_sack.hpp"
 #include "libdnf5/transaction/transaction_history.hpp"
 
+#include <libdnf5/utils/locker.hpp>
+
 
 namespace libdnf5::module {
 
@@ -115,6 +117,25 @@ class LIBDNF_API Base {
     /// Calling the method for the second time result in throwing an exception
     void setup();
 
+    /// Acquire an advisory lock on the installroot's system repository.
+    /// The lock will be automatically released when Base goes out of scope, or manually when unlock_system_repo is called.
+    /// A WRITE lock can be downgraded to a READ lock but a READ lock cannot be upgraded to a WRITE lock.
+    /// Should be called before the system repo is loaded, and the lock should be held until all transactions are
+    /// complete and other processes can safely re-read the RPMDB and resolve transactions.
+    /// @throw libdnf5::SystemError if an unexpected error occurs when locking
+    /// @return true if acquiring the lock succeeded, false otherwise
+    bool lock_system_repo(
+        libdnf5::utils::LockAccess access = libdnf5::utils::LockAccess::WRITE,
+        libdnf5::utils::LockBlocking blocking = libdnf5::utils::LockBlocking::NON_BLOCKING);
+
+    /// Release the lock obtained by lock_system_repo.
+    /// Idempotent. No-op if there is currently no lock.
+    /// @throw libdnf5::SystemError if an unexpected error occurs when unlocking
+    void unlock_system_repo();
+
+    /// Get a pointer to the lock on the system repo, or nullptr if no lock exists.
+    const libdnf5::utils::Locker * get_system_repo_lock() const noexcept;
+
     /// Returns true when setup() (mandatory method in many workflows) was already called
     bool is_initialized();