Merge pull request #674 from cgwalters/karg-cleanup-2

kargs: More cleanups

Author: cgwalters

lib/src/cli.rs

@@ -541,7 +541,6 @@ async fn upgrade(opts: UpgradeOpts) -> Result<()> {
         }
     } else {
         let fetched = crate::deploy::pull(repo, imgref, opts.quiet).await?;
-        let kargs = crate::kargs::get_kargs(repo, &booted_deployment, fetched.as_ref())?;
         let staged_digest = staged_image.as_ref().map(|s| s.image_digest.as_str());
         let fetched_digest = fetched.manifest_digest.as_str();
         tracing::debug!("staged: {staged_digest:?}");
@@ -563,10 +562,7 @@ async fn upgrade(opts: UpgradeOpts) -> Result<()> {
             println!("No update available.")
         } else {
             let osname = booted_deployment.osname();
-            let mut opts = ostree::SysrootDeployTreeOpts::default();
-            let kargs: Vec<&str> = kargs.iter().map(|s| s.as_str()).collect();
-            opts.override_kernel_argv = Some(kargs.as_slice());
-            crate::deploy::stage(sysroot, &osname, &fetched, &spec, Some(opts)).await?;
+            crate::deploy::stage(sysroot, &osname, &fetched, &spec).await?;
             changed = true;
             if let Some(prev) = booted_image.as_ref() {
                 if let Some(fetched_manifest) = fetched.get_manifest(repo)? {
@@ -638,7 +634,6 @@ async fn switch(opts: SwitchOpts) -> Result<()> {
     let new_spec = RequiredHostSpec::from_spec(&new_spec)?;
 
     let fetched = crate::deploy::pull(repo, &target, opts.quiet).await?;
-    let kargs = crate::kargs::get_kargs(repo, &booted_deployment, fetched.as_ref())?;
 
     if !opts.retain {
         // By default, we prune the previous ostree ref so it will go away after later upgrades
@@ -652,10 +647,7 @@ async fn switch(opts: SwitchOpts) -> Result<()> {
     }
 
     let stateroot = booted_deployment.osname();
-    let mut opts = ostree::SysrootDeployTreeOpts::default();
-    let kargs: Vec<&str> = kargs.iter().map(|s| s.as_str()).collect();
-    opts.override_kernel_argv = Some(kargs.as_slice());
-    crate::deploy::stage(sysroot, &stateroot, &fetched, &new_spec, Some(opts)).await?;
+    crate::deploy::stage(sysroot, &stateroot, &fetched, &new_spec).await?;
 
     Ok(())
 }
@@ -700,15 +692,11 @@ async fn edit(opts: EditOpts) -> Result<()> {
     }
 
     let fetched = crate::deploy::pull(repo, new_spec.image, opts.quiet).await?;
-    let kargs = crate::kargs::get_kargs(repo, &booted_deployment, fetched.as_ref())?;
 
     // TODO gc old layers here
 
     let stateroot = booted_deployment.osname();
-    let mut opts = ostree::SysrootDeployTreeOpts::default();
-    let kargs: Vec<&str> = kargs.iter().map(|s| s.as_str()).collect();
-    opts.override_kernel_argv = Some(kargs.as_slice());
-    crate::deploy::stage(sysroot, &stateroot, &fetched, &new_spec, Some(opts)).await?;
+    crate::deploy::stage(sysroot, &stateroot, &fetched, &new_spec).await?;
 
     Ok(())
 }

lib/src/deploy.rs

@@ -327,10 +327,26 @@ async fn deploy(
     stateroot: &str,
     image: &ImageState,
     origin: &glib::KeyFile,
-    opts: Option<ostree::SysrootDeployTreeOpts<'_>>,
 ) -> Result<Deployment> {
     let stateroot = Some(stateroot);
-    let opts = opts.unwrap_or_default();
+    let mut opts = ostree::SysrootDeployTreeOpts::default();
+    // Compute the kernel argument overrides. In practice today this API is always expecting
+    // a merge deployment. The kargs code also always looks at the booted root (which
+    // is a distinct minor issue, but not super important as right now the install path
+    // doesn't use this API).
+    let override_kargs = if let Some(deployment) = merge_deployment {
+        Some(crate::kargs::get_kargs(sysroot, &deployment, image)?)
+    } else {
+        None
+    };
+    // Because the C API expects a Vec<&str>, we need to generate a new Vec<>
+    // that borrows.
+    let override_kargs = override_kargs
+        .as_deref()
+        .map(|v| v.iter().map(|s| s.as_str()).collect::<Vec<_>>());
+    if let Some(kargs) = override_kargs.as_deref() {
+        opts.override_kernel_argv = Some(&kargs);
+    }
     // Copy to move into thread
     let cancellable = gio::Cancellable::NONE;
     return sysroot
@@ -364,7 +380,6 @@ pub(crate) async fn stage(
     stateroot: &str,
     image: &ImageState,
     spec: &RequiredHostSpec<'_>,
-    opts: Option<ostree::SysrootDeployTreeOpts<'_>>,
 ) -> Result<()> {
     let merge_deployment = sysroot.merge_deployment(Some(stateroot));
     let origin = origin_from_imageref(spec.image)?;
@@ -374,7 +389,6 @@ pub(crate) async fn stage(
         stateroot,
         image,
         &origin,
-        opts,
     )
     .await?;
     crate::deploy::cleanup(sysroot).await?;

lib/src/kargs.rs

@@ -1,6 +1,5 @@
 use anyhow::{Context, Result};
 use camino::Utf8Path;
-use cap_std_ext::cap_std;
 use cap_std_ext::cap_std::fs::Dir;
 use cap_std_ext::dirext::CapStdExtDirExt;
 use ostree::gio;
@@ -9,6 +8,7 @@ use ostree_ext::ostree::Deployment;
 use ostree_ext::prelude::Cast;
 use ostree_ext::prelude::FileEnumeratorExt;
 use ostree_ext::prelude::FileExt;
+use ostree_ext::sysroot::SysrootLock;
 use serde::Deserialize;
 
 use crate::deploy::ImageState;
@@ -101,25 +101,26 @@ fn get_kargs_from_ostree(
 /// karg, but applies the diff between the bootc karg files in /usr/lib/bootc/kargs.d
 /// between the booted deployment and the new one.
 pub(crate) fn get_kargs(
-    repo: &ostree::Repo,
-    booted_deployment: &Deployment,
+    sysroot: &SysrootLock,
+    merge_deployment: &Deployment,
     fetched: &ImageState,
 ) -> Result<Vec<String>> {
     let cancellable = gio::Cancellable::NONE;
-    let mut kargs: Vec<String> = vec![];
+    let repo = &sysroot.repo();
+    let mut kargs = vec![];
     let sys_arch = std::env::consts::ARCH;
 
-    // Get the running kargs of the booted system
-    if let Some(bootconfig) = ostree::Deployment::bootconfig(booted_deployment) {
+    // Get the kargs used for the merge in the bootloader config
+    if let Some(bootconfig) = ostree::Deployment::bootconfig(merge_deployment) {
         if let Some(options) = ostree::BootconfigParser::get(&bootconfig, "options") {
             let options = options.split_whitespace().map(|s| s.to_owned());
             kargs.extend(options);
         }
     };
 
-    // Get the kargs in kargs.d of the booted system
-    let root = &cap_std::fs::Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
-    let existing_kargs: Vec<String> = get_kargs_in_root(root, sys_arch)?;
+    // Get the kargs in kargs.d of the merge
+    let merge_root = &crate::utils::deployment_fd(sysroot, merge_deployment)?;
+    let existing_kargs = get_kargs_in_root(merge_root, sys_arch)?;
 
     // Get the kargs in kargs.d of the pending image
     let (fetched_tree, _) = repo.read_commit(fetched.ostree_commit.as_str(), cancellable)?;
@@ -138,16 +139,16 @@ pub(crate) fn get_kargs(
     let remote_kargs = get_kargs_from_ostree(repo, &fetched_tree, sys_arch)?;
 
     // get the diff between the existing and remote kargs
-    let mut added_kargs: Vec<String> = remote_kargs
+    let mut added_kargs = remote_kargs
         .clone()
         .into_iter()
         .filter(|item| !existing_kargs.contains(item))
-        .collect();
-    let removed_kargs: Vec<String> = existing_kargs
+        .collect::<Vec<_>>();
+    let removed_kargs = existing_kargs
         .clone()
         .into_iter()
         .filter(|item| !remote_kargs.contains(item))
-        .collect();
+        .collect::<Vec<_>>();
 
     tracing::debug!(
         "kargs: added={:?} removed={:?}",
@@ -179,6 +180,7 @@ fn parse_kargs_toml(contents: &str, sys_arch: &str) -> Result<Vec<String>> {
 
 #[cfg(test)]
 mod tests {
+    use cap_std_ext::cap_std;
     use fn_error_context::context;
     use rustix::fd::{AsFd, AsRawFd};
 

lib/src/utils.rs

@@ -1,9 +1,11 @@
 use std::future::Future;
 use std::io::Write;
+use std::os::fd::BorrowedFd;
 use std::process::Command;
 use std::time::Duration;
 
 use anyhow::{Context, Result};
+use cap_std_ext::cap_std::fs::Dir;
 use ostree::glib;
 use ostree_ext::container::SignatureSource;
 use ostree_ext::ostree;
@@ -21,6 +23,24 @@ pub(crate) fn origin_has_rpmostree_stuff(kf: &glib::KeyFile) -> bool {
     false
 }
 
+// Access the file descriptor for a sysroot
+#[allow(unsafe_code)]
+pub(crate) fn sysroot_fd(sysroot: &ostree::Sysroot) -> BorrowedFd {
+    unsafe { BorrowedFd::borrow_raw(sysroot.fd()) }
+}
+
+// Return a cap-std `Dir` type for a deployment.
+// TODO: in the future this should perhaps actually mount via composefs
+#[allow(unsafe_code)]
+pub(crate) fn deployment_fd(
+    sysroot: &ostree::Sysroot,
+    deployment: &ostree::Deployment,
+) -> Result<Dir> {
+    let sysroot_dir = &Dir::reopen_dir(&sysroot_fd(sysroot))?;
+    let dirpath = sysroot.deployment_dirpath(deployment);
+    sysroot_dir.open_dir(&dirpath).map_err(Into::into)
+}
+
 /// Given an mount option string list like foo,bar=baz,something=else,ro parse it and find
 /// the first entry like $optname=
 /// This will not match a bare `optname` without an equals.