From 5f17bc5c874beb80612d956a93d2d14456388210 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Tue, 9 Mar 2021 15:58:30 -0500 Subject: [PATCH] lockfile: Return LockfileConfig rather than Vec I don't like the use of `HY_GLOB` in the lockfile package matching. We have all the information in the Rust object, so it's silly to condense that to a single string in a hashmap. Fix this by returning the `LockfileConfig` object itself and then adding a function to fetch the list of locked packages. This allows the C++ side to see all the individual fields which makes filtering trivial. The next step is moving all the code which needs the lockfile to Rust. Then we can drop the shared `LockedPackage` type. (I did start on converting `find_locked_packages`, though it requires adding bindings for all the `HyQuery` stuff, which... isn't great (and also runs into the fact that `hy_query_run` needs to return a GPtrArray). I think instead of a 1:1 mapping, we'll probably want the libdnf-sys API wrappers to provide some sugar for the common paths.) --- rust/src/lib.rs | 13 ++++- rust/src/lockfile.rs | 64 ++++++++++++---------- src/app/rpmostree-compose-builtin-tree.cxx | 9 +-- src/libpriv/rpmostree-core-private.h | 8 ++- src/libpriv/rpmostree-core.cxx | 61 ++++++++++++--------- src/libpriv/rpmostree-core.h | 4 +- 6 files changed, 89 insertions(+), 70 deletions(-) diff --git a/rust/src/lib.rs b/rust/src/lib.rs index d96ba697..e9dbceb2 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -263,14 +263,25 @@ pub mod ffi { fn serialize_to_dir(&self, output_dir: &str) -> Result<()>; } + struct LockedPackage { + name: String, + evr: String, + arch: String, + digest: String, + } + // lockfile.rs extern "Rust" { - fn lockfile_read(filenames: &Vec) -> Result>; + type LockfileConfig; + + fn lockfile_read(filenames: &Vec) -> Result>; fn lockfile_write( filename: &str, packages: Pin<&mut CxxGObjectArray>, rpmmd_repos: Pin<&mut CxxGObjectArray>, ) -> Result<()>; + + fn get_locked_packages(&self) -> Result>; } // rpmutils.rs diff --git a/rust/src/lockfile.rs b/rust/src/lockfile.rs index 01995fff..6f33f8b5 100644 --- a/rust/src/lockfile.rs +++ b/rust/src/lockfile.rs @@ -12,7 +12,7 @@ pub use self::ffi::*; use crate::utils; -use anyhow::Result; +use anyhow::{anyhow, Result}; use chrono::prelude::*; use openat_ext::OpenatDirExt; use serde_derive::{Deserialize, Serialize}; @@ -23,6 +23,9 @@ use std::iter::Extend; use std::path::Path; use std::pin::Pin; +use crate::cxxrsutil::CxxResult; +use libdnf_sys::*; + /// Given a lockfile filename, parse it fn lockfile_parse>(filename: P) -> Result { let filename = filename.as_ref(); @@ -90,7 +93,7 @@ fn lockfile_parse_multiple>(filenames: &[P]) -> Result, metadata: Option, } @@ -120,25 +123,37 @@ enum LockedPackage { }, } -impl LockedPackage { - fn evra_glob(&self) -> String { - match self { - LockedPackage::Evr { evr, digest: _ } => format!("{}.*", evr), - LockedPackage::Evra { evra, digest: _ } => evra.into(), - } - } - fn digest(&self) -> String { - match self { - LockedPackage::Evr { evr: _, digest } => digest.clone().unwrap_or_default(), - LockedPackage::Evra { evra: _, digest } => digest.clone().unwrap_or_default(), - } - } -} - impl LockfileConfig { fn merge(&mut self, other: LockfileConfig) { self.packages.extend(other.packages); } + + pub(crate) fn get_locked_packages(&self) -> CxxResult> { + self.packages + .iter() + .map(|(k, v)| match v { + LockedPackage::Evr { evr, digest } => Ok(crate::ffi::LockedPackage { + name: k.clone(), + evr: evr.clone(), + arch: "".into(), + digest: digest.clone().unwrap_or_default(), + }), + LockedPackage::Evra { evra, digest } => { + let evr_arch: Vec<&str> = evra.rsplitn(2, '.').collect(); + if evr_arch.len() != 2 { + Err(anyhow!("package {} has malformed evra: {}", k, evra).into()) + } else { + Ok(crate::ffi::LockedPackage { + name: k.clone(), + evr: evr_arch[1].into(), + arch: evr_arch[0].into(), + digest: digest.clone().unwrap_or_default(), + }) + } + } + }) + .collect() + } } #[cfg(test)] @@ -238,19 +253,8 @@ mod tests { } } -use crate::cxxrsutil::CxxResult; -use crate::ffi::*; -use libdnf_sys::*; - -pub(crate) fn lockfile_read(filenames: &Vec) -> CxxResult> { - Ok(lockfile_parse_multiple(&filenames)? - .packages - .into_iter() - .map(|(k, v)| StringMapping { - k: format!("{}-{}", k, v.evra_glob()), - v: v.digest(), - }) - .collect()) +pub(crate) fn lockfile_read(filenames: &Vec) -> CxxResult> { + Ok(Box::new(lockfile_parse_multiple(&filenames)?)) } pub(crate) fn lockfile_write( diff --git a/src/app/rpmostree-compose-builtin-tree.cxx b/src/app/rpmostree-compose-builtin-tree.cxx index 6d24e1c5..0f1fb47f 100644 --- a/src/app/rpmostree-compose-builtin-tree.cxx +++ b/src/app/rpmostree-compose-builtin-tree.cxx @@ -704,14 +704,7 @@ rpm_ostree_compose_context_new (const char *treefile_pathstr, if (opt_lockfiles) { - rust::Vec lockfiles; - for (char **it = opt_lockfiles; it && *it; it++) - lockfiles.push_back(std::string(*it)); - auto mappings = rpmostreecxx::lockfile_read (lockfiles); - g_autoptr(GHashTable) map = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); - for (auto & mapping : mappings) - g_hash_table_insert (map, g_strdup (mapping.k.c_str()), g_strdup (mapping.v.c_str())); - rpmostree_context_set_vlockmap (self->corectx, map, opt_lockfile_strict); + rpmostree_context_set_lockfile (self->corectx, opt_lockfiles, opt_lockfile_strict); g_print ("Loaded lockfiles:\n %s\n", g_strjoinv ("\n ", opt_lockfiles)); } diff --git a/src/libpriv/rpmostree-core-private.h b/src/libpriv/rpmostree-core-private.h index 2b2fa262..f195a775 100644 --- a/src/libpriv/rpmostree-core-private.h +++ b/src/libpriv/rpmostree-core-private.h @@ -20,6 +20,8 @@ #pragma once +#include + #include "libglnx.h" #include "rpmostree-rojig-core.h" #include "rpmostree-core.h" @@ -77,8 +79,8 @@ struct _RpmOstreeContext { GHashTable *pkgs_to_remove; /* pkgname --> gv_nevra */ GHashTable *pkgs_to_replace; /* new gv_nevra --> old gv_nevra */ - GHashTable *vlockmap; /* nevra --> repochecksum */ - gboolean vlockmap_strict; + std::optional> lockfile; + gboolean lockfile_strict; GLnxTmpDir tmpdir; @@ -89,4 +91,4 @@ struct _RpmOstreeContext { GLnxTmpDir repo_tmpdir; /* Used to assemble+commit if no base rootfs provided */ }; -G_END_DECLS \ No newline at end of file +G_END_DECLS diff --git a/src/libpriv/rpmostree-core.cxx b/src/libpriv/rpmostree-core.cxx index c9772377..0e6a75e7 100644 --- a/src/libpriv/rpmostree-core.cxx +++ b/src/libpriv/rpmostree-core.cxx @@ -355,8 +355,6 @@ rpmostree_context_finalize (GObject *object) g_clear_pointer (&rctx->pkgs_to_remove, g_hash_table_unref); g_clear_pointer (&rctx->pkgs_to_replace, g_hash_table_unref); - g_clear_pointer (&rctx->vlockmap, g_hash_table_unref); - (void)glnx_tmpdir_delete (&rctx->tmpdir, NULL, NULL); (void)glnx_tmpdir_delete (&rctx->repo_tmpdir, NULL, NULL); @@ -796,7 +794,7 @@ rpmostree_context_setup (RpmOstreeContext *self, /* only enable lockfile-repos if we actually have a lockfile so we don't even waste * time fetching metadata */ - if (self->vlockmap) + if (self->lockfile) { g_autofree char **enabled_lockfile_repos = NULL; if (g_variant_dict_lookup (self->spec->dict, "lockfile-repos", "^a&s", &enabled_lockfile_repos)) @@ -1912,17 +1910,20 @@ static GPtrArray* find_locked_packages (RpmOstreeContext *self, GError **error) { - g_assert (self->vlockmap); + g_assert (self->lockfile); DnfContext *dnfctx = self->dnfctx; DnfSack *sack = dnf_context_get_sack (dnfctx); g_autoptr(GPtrArray) pkgs = g_ptr_array_new_with_free_func ((GDestroyNotify)g_object_unref); - GLNX_HASH_TABLE_FOREACH_KV (self->vlockmap, const char*, nevra, const char*, chksum) + auto locked_pkgs = (*self->lockfile)->get_locked_packages(); + for (auto & pkg : locked_pkgs) { - g_assert (chksum); hy_autoquery HyQuery query = hy_query_create (sack); - hy_query_filter (query, HY_PKG_NEVRA, HY_GLOB, nevra); + hy_query_filter (query, HY_PKG_NAME, HY_EQ, pkg.name.c_str()); + hy_query_filter (query, HY_PKG_EVR, HY_EQ, pkg.evr.c_str()); + if (pkg.arch.length() > 0) + hy_query_filter (query, HY_PKG_ARCH, HY_EQ, pkg.arch.c_str()); g_autoptr(GPtrArray) matches = hy_query_run (query); gboolean at_least_one = FALSE; @@ -1930,7 +1931,7 @@ find_locked_packages (RpmOstreeContext *self, for (guint i = 0; i < matches->len; i++) { auto match = static_cast(matches->pdata[i]); - if (!*chksum) + if (pkg.digest.length() == 0) { /* we could optimize this path outside the loop in the future using the * g_ptr_array_extend_and_steal API, though that's still too new for e.g. el8 */ @@ -1940,7 +1941,7 @@ find_locked_packages (RpmOstreeContext *self, else { auto repodata_chksum = rpmostreecxx::get_repodata_chksum_repr(*match); - if (!g_str_equal (chksum, repodata_chksum.c_str())) + if (pkg.digest != repodata_chksum) /* we're comparing two rust::String here */ n_checksum_mismatches++; else { @@ -1950,10 +1951,17 @@ find_locked_packages (RpmOstreeContext *self, } } if (!at_least_one) - return (GPtrArray*)glnx_null_throw (error, "Couldn't find locked package '%s'%s%s " - "(pkgs matching NEVRA: %d; mismatched checksums: %d)", - nevra, *chksum ? " with checksum " : "", - *chksum ? chksum : "", matches->len, n_checksum_mismatches); + { + g_autofree char *spec = + g_strdup_printf ("%s-%s%s%s", pkg.name.c_str(), pkg.evr.c_str(), + pkg.arch.length() > 0 ? "." : "", + pkg.arch.length() > 0 ? pkg.arch.c_str() : ""); + return (GPtrArray*)glnx_null_throw (error, "Couldn't find locked package '%s'%s%s " + "(pkgs matching NEVRA: %d; mismatched checksums: %d)", + spec, pkg.digest.length() > 0 ? " with checksum " : "", + pkg.digest.length() > 0 ? pkg.digest.c_str() : "", + matches->len, n_checksum_mismatches); + } } return util::move_nullify (pkgs); @@ -2015,7 +2023,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, g_assert_cmpint (g_strv_length (removed_base_pkgnames), ==, 0); } - if (self->vlockmap) + if (self->lockfile) { /* we only support pure installs for now (compose case) */ g_assert (!self->rojig_pure); @@ -2085,7 +2093,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, * uninstall. We don't want to mix those two steps, otherwise we might confuse libdnf, * see: https://github.com/rpm-software-management/libdnf/issues/700 */ - if (self->vlockmap != NULL) + if (self->lockfile) { /* first, find our locked pkgs in the rpmmd */ g_autoptr(GPtrArray) locked_pkgs = find_locked_packages (self, error); @@ -2097,7 +2105,7 @@ rpmostree_context_prepare (RpmOstreeContext *self, for (guint i = 0; i < locked_pkgs->len; i++) dnf_packageset_add (locked_pset, static_cast(locked_pkgs->pdata[i])); - if (self->vlockmap_strict) + if (self->lockfile_strict) { /* In strict mode, we basically *only* want locked packages to be considered, so * exclude everything else. Note we still don't directly do `hy_goal_install` @@ -2135,13 +2143,11 @@ rpmostree_context_prepare (RpmOstreeContext *self, /* map of all packages with names found in the lockfile */ DnfPackageSet *named_pkgs = dnf_packageset_new (sack); Map *named_pkgs_map = dnf_packageset_get_map (named_pkgs); - GLNX_HASH_TABLE_FOREACH (self->vlockmap, const char*, nevra) + auto locked_pkgs = (*self->lockfile)->get_locked_packages(); + for (auto & pkg : locked_pkgs) { - g_autofree char *name = NULL; - if (!rpmostree_decompose_nevra (nevra, &name, NULL, NULL, NULL, NULL, error)) - return FALSE; hy_autoquery HyQuery query = hy_query_create (sack); - hy_query_filter (query, HY_PKG_NAME, HY_EQ, name); + hy_query_filter (query, HY_PKG_NAME, HY_EQ, pkg.name.c_str()); DnfPackageSet *pset = hy_query_run_set (query); map_or (named_pkgs_map, dnf_packageset_get_map (pset)); dnf_packageset_free (pset); @@ -2315,13 +2321,16 @@ rpmostree_context_get_packages_to_import (RpmOstreeContext *self) /* Note this must be called *before* rpmostree_context_setup(). */ void -rpmostree_context_set_vlockmap (RpmOstreeContext *self, - GHashTable *map, +rpmostree_context_set_lockfile (RpmOstreeContext *self, + char **lockfiles, gboolean strict) { - g_clear_pointer (&self->vlockmap, (GDestroyNotify)g_hash_table_unref); - self->vlockmap = g_hash_table_ref (map); - self->vlockmap_strict = strict; + g_assert (!self->lockfile); + rust::Vec rs_lockfiles; + for (char **it = lockfiles; it && *it; it++) + rs_lockfiles.push_back(std::string(*it)); + self->lockfile = rpmostreecxx::lockfile_read(rs_lockfiles); + self->lockfile_strict = strict; } /* XXX: push this into libdnf */ diff --git a/src/libpriv/rpmostree-core.h b/src/libpriv/rpmostree-core.h index 9f57e3e1..419d6203 100644 --- a/src/libpriv/rpmostree-core.h +++ b/src/libpriv/rpmostree-core.h @@ -182,8 +182,8 @@ gboolean rpmostree_context_set_packages (RpmOstreeContext *self, GPtrArray *rpmostree_context_get_packages_to_import (RpmOstreeContext *self); void -rpmostree_context_set_vlockmap (RpmOstreeContext *self, - GHashTable *map, +rpmostree_context_set_lockfile (RpmOstreeContext *self, + char **lockfiles, gboolean strict); gboolean rpmostree_download_packages (GPtrArray *packages,