From 7d91ed9d17b5156588aa233e761aab055777da44 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 10 Jul 2024 20:38:26 -0400 Subject: [PATCH 1/3] deploy: Centralize karg computation I happened to be looking at this code for an unrelated reason. We had 3 copies of the function calling karg computation. But that can just be pushed to a much lower level, right before we tell ostree what to do. Signed-off-by: Colin Walters --- lib/src/cli.rs | 18 +++--------------- lib/src/deploy.rs | 26 ++++++++++++++++++++++---- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 5bd17d0c4..ee4dcb218 100644 --- a/lib/src/cli.rs +++ b/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(()) } diff --git a/lib/src/deploy.rs b/lib/src/deploy.rs index abc8d7969..7d50a31cc 100644 --- a/lib/src/deploy.rs +++ b/lib/src/deploy.rs @@ -327,10 +327,30 @@ async fn deploy( stateroot: &str, image: &ImageState, origin: &glib::KeyFile, - opts: Option>, ) -> Result { 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.repo(), + &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::>()); + 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 +384,6 @@ pub(crate) async fn stage( stateroot: &str, image: &ImageState, spec: &RequiredHostSpec<'_>, - opts: Option>, ) -> Result<()> { let merge_deployment = sysroot.merge_deployment(Some(stateroot)); let origin = origin_from_imageref(spec.image)?; @@ -374,7 +393,6 @@ pub(crate) async fn stage( stateroot, image, &origin, - opts, ) .await?; crate::deploy::cleanup(sysroot).await?; From 46516cd659c3937e474380143cf7ba4ded88fda8 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 10 Jul 2024 20:38:26 -0400 Subject: [PATCH 2/3] kargs: Use merge deployment This is in general for us right now a no-op, but is closer to technically correct. In ostree the "merge deployment" concept started out as "deployment we use for the original /etc" which *may be different* from the booted deployment. For example, when one wants to do a "factory reset", we want to start from a fresh /etc. The other important case here is when we're doing a fresh install (or into a new stateroot) there may be no merge deployment at all! It wouldn't be correct to look at `/`. We only sidestep this issue right now because the install logic bypasses this to directly gather kargs...and doesn't use the same `deploy` API as inplace updates. But we want to get closer to doing things that way. Signed-off-by: Colin Walters --- lib/src/deploy.rs | 6 +----- lib/src/kargs.rs | 18 ++++++++++-------- lib/src/utils.rs | 20 ++++++++++++++++++++ 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/lib/src/deploy.rs b/lib/src/deploy.rs index 7d50a31cc..ff66562c3 100644 --- a/lib/src/deploy.rs +++ b/lib/src/deploy.rs @@ -335,11 +335,7 @@ async fn deploy( // 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.repo(), - &deployment, - image, - )?) + Some(crate::kargs::get_kargs(sysroot, &deployment, image)?) } else { None }; diff --git a/lib/src/kargs.rs b/lib/src/kargs.rs index 5dac9158e..eb87b05a7 100644 --- a/lib/src/kargs.rs +++ b/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> { let cancellable = gio::Cancellable::NONE; + let repo = &sysroot.repo(); let mut kargs: Vec = 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 = 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: Vec = 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)?; @@ -179,6 +180,7 @@ fn parse_kargs_toml(contents: &str, sys_arch: &str) -> Result> { #[cfg(test)] mod tests { + use cap_std_ext::cap_std; use fn_error_context::context; use rustix::fd::{AsFd, AsRawFd}; diff --git a/lib/src/utils.rs b/lib/src/utils.rs index bc50fdfeb..45c72fdbb 100644 --- a/lib/src/utils.rs +++ b/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 { + 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. From 5452ae66f94f00bb5641acaeb963a26f5049bfbd Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 11 Jul 2024 09:15:40 -0400 Subject: [PATCH 3/3] kargs: Use type inference No need for explicit types in most of these places. Signed-off-by: Colin Walters --- lib/src/kargs.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/src/kargs.rs b/lib/src/kargs.rs index eb87b05a7..8840dfbef 100644 --- a/lib/src/kargs.rs +++ b/lib/src/kargs.rs @@ -107,7 +107,7 @@ pub(crate) fn get_kargs( ) -> Result> { let cancellable = gio::Cancellable::NONE; let repo = &sysroot.repo(); - let mut kargs: Vec = vec![]; + let mut kargs = vec![]; let sys_arch = std::env::consts::ARCH; // Get the kargs used for the merge in the bootloader config @@ -120,7 +120,7 @@ pub(crate) fn get_kargs( // Get the kargs in kargs.d of the merge let merge_root = &crate::utils::deployment_fd(sysroot, merge_deployment)?; - let existing_kargs: Vec = get_kargs_in_root(merge_root, sys_arch)?; + 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)?; @@ -139,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 = remote_kargs + let mut added_kargs = remote_kargs .clone() .into_iter() .filter(|item| !existing_kargs.contains(item)) - .collect(); - let removed_kargs: Vec = existing_kargs + .collect::>(); + let removed_kargs = existing_kargs .clone() .into_iter() .filter(|item| !remote_kargs.contains(item)) - .collect(); + .collect::>(); tracing::debug!( "kargs: added={:?} removed={:?}",