diff --git a/crates/kernel_cmdline/src/bytes.rs b/crates/kernel_cmdline/src/bytes.rs index 694f7efb8..73d062903 100644 --- a/crates/kernel_cmdline/src/bytes.rs +++ b/crates/kernel_cmdline/src/bytes.rs @@ -18,6 +18,9 @@ use anyhow::Result; #[derive(Clone, Debug, Default)] pub struct Cmdline<'a>(Cow<'a, [u8]>); +/// An owned Cmdline. Alias for `Cmdline<'static>`. +pub type CmdlineOwned = Cmdline<'static>; + impl<'a, T: AsRef<[u8]> + ?Sized> From<&'a T> for Cmdline<'a> { /// Creates a new `Cmdline` from any type that can be referenced as bytes. /// @@ -27,7 +30,7 @@ impl<'a, T: AsRef<[u8]> + ?Sized> From<&'a T> for Cmdline<'a> { } } -impl<'a> From> for Cmdline<'a> { +impl From> for CmdlineOwned { /// Creates a new `Cmdline` from an owned `Vec`. fn from(input: Vec) -> Self { Self(Cow::Owned(input)) @@ -85,7 +88,7 @@ impl<'a> Cmdline<'a> { /// Creates a new empty owned `Cmdline`. /// /// This is equivalent to `Cmdline::default()` but makes ownership explicit. - pub fn new() -> Cmdline<'static> { + pub fn new() -> CmdlineOwned { Cmdline::default() } @@ -294,6 +297,29 @@ impl<'a> Cmdline<'a> { removed } + /// Remove all parameters that exactly match the given parameter + /// from the command line + /// + /// Returns `true` if parameter(s) were removed. + pub fn remove_exact(&mut self, param: &Parameter) -> bool { + let mut removed = false; + let mut new_params = Vec::new(); + + for p in self.iter() { + if p == *param { + removed = true; + } else { + new_params.push(p.parameter); + } + } + + if removed { + self.0 = Cow::Owned(new_params.join(b" ".as_slice())); + } + + removed + } + #[cfg(test)] pub(crate) fn is_owned(&self) -> bool { matches!(self.0, Cow::Owned(_)) @@ -647,8 +673,8 @@ mod tests { assert_eq!(kargs.iter().next(), None); assert!(kargs.is_owned()); - // Verify we can store it in a 'static context - let _static_kargs: Cmdline<'static> = Cmdline::new(); + // Verify we can store it in an owned ('static) context + let _static_kargs: CmdlineOwned = Cmdline::new(); } #[test] @@ -904,6 +930,25 @@ mod tests { assert_eq!(iter.next(), None); } + #[test] + fn test_remove_exact() { + let mut kargs = Cmdline::from(b"foo foo=bar foo=baz"); + + // remove existing + assert!(kargs.remove_exact(¶m("foo=bar"))); + let mut iter = kargs.iter(); + assert_eq!(iter.next(), Some(param("foo"))); + assert_eq!(iter.next(), Some(param("foo=baz"))); + assert_eq!(iter.next(), None); + + // doesn't exist? returns false and doesn't modify anything + assert!(!kargs.remove_exact(¶m("foo=wuz"))); + iter = kargs.iter(); + assert_eq!(iter.next(), Some(param("foo"))); + assert_eq!(iter.next(), Some(param("foo=baz"))); + assert_eq!(iter.next(), None); + } + #[test] fn test_extend() { let mut kargs = Cmdline::from(b"foo=bar baz"); diff --git a/crates/kernel_cmdline/src/utf8.rs b/crates/kernel_cmdline/src/utf8.rs index 8713bbeb3..5403154bb 100644 --- a/crates/kernel_cmdline/src/utf8.rs +++ b/crates/kernel_cmdline/src/utf8.rs @@ -17,6 +17,9 @@ use anyhow::Result; #[derive(Clone, Debug, Default)] pub struct Cmdline<'a>(bytes::Cmdline<'a>); +/// An owned `Cmdline`. Alias for `Cmdline<'static>`. +pub type CmdlineOwned = Cmdline<'static>; + impl<'a, T: AsRef + ?Sized> From<&'a T> for Cmdline<'a> { /// Creates a new `Cmdline` from any type that can be referenced as `str`. /// @@ -26,7 +29,7 @@ impl<'a, T: AsRef + ?Sized> From<&'a T> for Cmdline<'a> { } } -impl From for Cmdline<'static> { +impl From for CmdlineOwned { /// Creates a new `Cmdline` from a `String`. /// /// Takes ownership of input and maintains it for internal owned data. @@ -72,7 +75,7 @@ impl<'a> Cmdline<'a> { /// Creates a new empty owned `Cmdline`. /// /// This is equivalent to `Cmdline::default()` but makes ownership explicit. - pub fn new() -> Cmdline<'static> { + pub fn new() -> CmdlineOwned { Cmdline::default() } @@ -191,6 +194,14 @@ impl<'a> Cmdline<'a> { self.0.remove(&key.0) } + /// Remove all parameters that exactly match the given parameter + /// from the command line + /// + /// Returns `true` if parameter(s) were removed. + pub fn remove_exact(&mut self, param: &Parameter) -> bool { + self.0.remove_exact(¶m.0) + } + #[cfg(test)] pub(crate) fn is_owned(&self) -> bool { self.0.is_owned() @@ -569,8 +580,8 @@ mod tests { assert_eq!(kargs.iter().next(), None); assert!(kargs.is_owned()); - // Verify we can store it in a 'static context - let _static_kargs: Cmdline<'static> = Cmdline::new(); + // Verify we can store it in an owned ('static) context + let _static_kargs: CmdlineOwned = Cmdline::new(); } #[test] @@ -829,6 +840,25 @@ mod tests { assert_eq!(iter.next(), None); } + #[test] + fn test_remove_exact() { + let mut kargs = Cmdline::from("foo foo=bar foo=baz"); + + // remove existing + assert!(kargs.remove_exact(¶m("foo=bar"))); + let mut iter = kargs.iter(); + assert_eq!(iter.next(), Some(param("foo"))); + assert_eq!(iter.next(), Some(param("foo=baz"))); + assert_eq!(iter.next(), None); + + // doesn't exist? returns false and doesn't modify anything + assert!(!kargs.remove_exact(¶m("foo=wuz"))); + iter = kargs.iter(); + assert_eq!(iter.next(), Some(param("foo"))); + assert_eq!(iter.next(), Some(param("foo=baz"))); + assert_eq!(iter.next(), None); + } + #[test] fn test_extend() { let mut kargs = Cmdline::from("foo=bar baz"); diff --git a/crates/lib/src/bootc_kargs.rs b/crates/lib/src/bootc_kargs.rs index 85fcac10b..af709f1af 100644 --- a/crates/lib/src/bootc_kargs.rs +++ b/crates/lib/src/bootc_kargs.rs @@ -1,5 +1,6 @@ //! This module handles the bootc-owned kernel argument lists in `/usr/lib/bootc/kargs.d`. use anyhow::{Context, Result}; +use bootc_kernel_cmdline::utf8::{Cmdline, CmdlineOwned}; use camino::Utf8Path; use cap_std_ext::cap_std::fs::Dir; use cap_std_ext::cap_std::fs_utf8::Dir as DirUtf8; @@ -20,11 +21,11 @@ use crate::store::Storage; const KARGS_PATH: &str = "usr/lib/bootc/kargs.d"; /// The default root filesystem mount specification. -pub(crate) const ROOT: &str = "root="; +pub(crate) const ROOT_KEY: &str = "root"; /// This is used by dracut. pub(crate) const INITRD_ARG_PREFIX: &str = "rd."; /// The kernel argument for configuring the rootfs flags. -pub(crate) const ROOTFLAGS: &str = "rootflags="; +pub(crate) const ROOTFLAGS_KEY: &str = "rootflags"; /// The kargs.d configuration file. #[derive(Deserialize)] @@ -46,31 +47,36 @@ impl Config { /// Load and parse all bootc kargs.d files in the specified root, returning /// a combined list. -pub(crate) fn get_kargs_in_root(d: &Dir, sys_arch: &str) -> Result> { +pub(crate) fn get_kargs_in_root(d: &Dir, sys_arch: &str) -> Result { // If the directory doesn't exist, that's OK. let Some(d) = d.open_dir_optional(KARGS_PATH)?.map(DirUtf8::from_cap_std) else { return Ok(Default::default()); }; - let mut ret = Vec::new(); + let mut ret = Cmdline::new(); let entries = d.filenames_filtered_sorted(|_, name| Config::filename_matches(name))?; for name in entries { let buf = d.read_to_string(&name)?; - let kargs = parse_kargs_toml(&buf, sys_arch).with_context(|| format!("Parsing {name}"))?; - ret.extend(kargs) + if let Some(kargs) = + parse_kargs_toml(&buf, sys_arch).with_context(|| format!("Parsing {name}"))? + { + ret.extend(&kargs) + } } Ok(ret) } -pub(crate) fn root_args_from_cmdline<'a>(cmdline: &'a [&str]) -> Vec<&'a str> { - cmdline - .iter() - .filter(|arg| { - arg.starts_with(ROOT) - || arg.starts_with(ROOTFLAGS) - || arg.starts_with(INITRD_ARG_PREFIX) - }) - .copied() - .collect() +pub(crate) fn root_args_from_cmdline(cmdline: &Cmdline) -> CmdlineOwned { + let mut result = Cmdline::new(); + for param in cmdline { + let key = param.key(); + if key == ROOT_KEY.into() + || key == ROOTFLAGS_KEY.into() + || key.starts_with(INITRD_ARG_PREFIX) + { + result.add(¶m); + } + } + result } /// Load kargs.d files from the target ostree commit root @@ -78,7 +84,7 @@ pub(crate) fn get_kargs_from_ostree_root( repo: &ostree::Repo, root: &ostree::RepoFile, sys_arch: &str, -) -> Result> { +) -> Result { let kargsd = root.resolve_relative_path(KARGS_PATH); let kargsd = kargsd.downcast_ref::().expect("downcast"); if !kargsd.query_exists(gio::Cancellable::NONE) { @@ -92,12 +98,12 @@ fn get_kargs_from_ostree( repo: &ostree::Repo, fetched_tree: &ostree::RepoFile, sys_arch: &str, -) -> Result> { +) -> Result { let cancellable = gio::Cancellable::NONE; let queryattrs = "standard::name,standard::type"; let queryflags = gio::FileQueryInfoFlags::NOFOLLOW_SYMLINKS; let fetched_iter = fetched_tree.enumerate_children(queryattrs, queryflags, cancellable)?; - let mut ret = Vec::new(); + let mut ret = Cmdline::new(); while let Some(fetched_info) = fetched_iter.next_file(cancellable)? { // only read and parse the file if it is a toml file let name = fetched_info.name(); @@ -119,9 +125,11 @@ fn get_kargs_from_ostree( let mut reader = ostree_ext::prelude::InputStreamExtManual::into_read(file_content.unwrap()); let s = std::io::read_to_string(&mut reader)?; - let parsed_kargs = - parse_kargs_toml(&s, sys_arch).with_context(|| format!("Parsing {name}"))?; - ret.extend(parsed_kargs); + if let Some(parsed_kargs) = + parse_kargs_toml(&s, sys_arch).with_context(|| format!("Parsing {name}"))? + { + ret.extend(&parsed_kargs); + } } Ok(ret) } @@ -133,20 +141,19 @@ pub(crate) fn get_kargs( sysroot: &Storage, merge_deployment: &Deployment, fetched: &ImageState, -) -> Result> { +) -> Result { let cancellable = gio::Cancellable::NONE; let ostree = sysroot.get_ostree()?; let repo = &ostree.repo(); - let mut kargs = vec![]; let sys_arch = std::env::consts::ARCH; // 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); - } - }; + let mut kargs = ostree::Deployment::bootconfig(merge_deployment) + .and_then(|bootconfig| { + ostree::BootconfigParser::get(&bootconfig, "options") + .map(|options| Cmdline::from(options.to_string())) + }) + .unwrap_or_default(); // Get the kargs in kargs.d of the merge let merge_root = &crate::utils::deployment_fd(ostree, merge_deployment)?; @@ -161,24 +168,22 @@ pub(crate) fn get_kargs( // A special case: if there's no kargs.d directory in the pending (fetched) image, // then we can just use the combined current kargs + kargs from booted if !fetched_tree.query_exists(cancellable) { - kargs.extend(existing_kargs); + kargs.extend(&existing_kargs); return Ok(kargs); } // Fetch the kernel arguments from the new root 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 = remote_kargs - .clone() - .into_iter() - .filter(|item| !existing_kargs.contains(item)) - .collect::>(); - let removed_kargs = existing_kargs - .clone() - .into_iter() - .filter(|item| !remote_kargs.contains(item)) - .collect::>(); + // Calculate the diff between the existing and remote kargs + let added_kargs: Vec<_> = remote_kargs + .iter() + .filter(|item| !existing_kargs.iter().any(|existing| *item == existing)) + .collect(); + let removed_kargs: Vec<_> = existing_kargs + .iter() + .filter(|item| !remote_kargs.iter().any(|remote| *item == remote)) + .collect(); tracing::debug!( "kargs: added={:?} removed={:?}", @@ -186,9 +191,13 @@ pub(crate) fn get_kargs( removed_kargs ); - // apply the diff to the system kargs - kargs.retain(|x| !removed_kargs.contains(x)); - kargs.append(&mut added_kargs); + // Apply the diff to the system kargs + for arg in &removed_kargs { + kargs.remove_exact(arg); + } + for arg in &added_kargs { + kargs.add(arg); + } Ok(kargs) } @@ -196,7 +205,7 @@ pub(crate) fn get_kargs( /// This parses a bootc kargs.d toml file, returning the resulting /// vector of kernel arguments. Architecture matching is performed using /// `sys_arch`. -fn parse_kargs_toml(contents: &str, sys_arch: &str) -> Result> { +fn parse_kargs_toml(contents: &str, sys_arch: &str) -> Result> { let de: Config = toml::from_str(contents)?; // if arch specified, apply kargs only if the arch matches // if arch not specified, apply kargs unconditionally @@ -204,7 +213,11 @@ fn parse_kargs_toml(contents: &str, sys_arch: &str) -> Result> { .match_architectures .map(|arches| arches.iter().any(|s| s == sys_arch)) .unwrap_or(true); - let r = if matched { de.kargs } else { Vec::new() }; + let r = if matched { + Some(Cmdline::from(de.kargs.join(" "))) + } else { + None + }; Ok(r) } @@ -216,17 +229,23 @@ mod tests { use super::*; + fn assert_cmdline_eq(cmdline: &Cmdline, expected_params: &[&str]) { + let actual_params: Vec<_> = cmdline.iter_str().collect(); + assert_eq!(actual_params, expected_params); + } + #[test] /// Verify that kargs are only applied to supported architectures fn test_arch() { // no arch specified, kargs ensure that kargs are applied unconditionally let sys_arch = "x86_64"; let file_content = r##"kargs = ["console=tty0", "nosmt"]"##.to_string(); - let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap(); - assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]); + let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap().unwrap(); + assert_cmdline_eq(&parsed_kargs, &["console=tty0", "nosmt"]); + let sys_arch = "aarch64"; - let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap(); - assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]); + let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap().unwrap(); + assert_cmdline_eq(&parsed_kargs, &["console=tty0", "nosmt"]); // one arch matches and one doesn't, ensure that kargs are only applied for the matching arch let sys_arch = "aarch64"; @@ -235,13 +254,13 @@ match-architectures = ["x86_64"] "## .to_string(); let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap(); - assert_eq!(parsed_kargs, [] as [String; 0]); + assert!(parsed_kargs.is_none()); let file_content = r##"kargs = ["console=tty0", "nosmt"] match-architectures = ["aarch64"] "## .to_string(); - let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap(); - assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]); + let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap().unwrap(); + assert_cmdline_eq(&parsed_kargs, &["console=tty0", "nosmt"]); // multiple arch specified, ensure that kargs are applied to both archs let sys_arch = "x86_64"; @@ -249,11 +268,12 @@ match-architectures = ["aarch64"] match-architectures = ["x86_64", "aarch64"] "## .to_string(); - let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap(); - assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]); + let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap().unwrap(); + assert_cmdline_eq(&parsed_kargs, &["console=tty0", "nosmt"]); + let sys_arch = "aarch64"; - let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap(); - assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]); + let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap().unwrap(); + assert_cmdline_eq(&parsed_kargs, &["console=tty0", "nosmt"]); } #[test] @@ -285,18 +305,18 @@ match-architectures = ["x86_64", "aarch64"] let td = cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?; // No directory - assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().len(), 0); + assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().iter().count(), 0); // Empty directory td.create_dir_all("usr/lib/bootc/kargs.d")?; - assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().len(), 0); + assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().iter().count(), 0); // Non-toml file td.write("usr/lib/bootc/kargs.d/somegarbage", "garbage")?; - assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().len(), 0); + assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().iter().count(), 0); write_test_kargs(&td)?; let args = get_kargs_in_root(&td, "x86_64").unwrap(); - similar_asserts::assert_eq!(args, ["console=tty0", "nosmt", "console=ttyS1"]); + assert_cmdline_eq(&args, &["console=tty0", "nosmt", "console=ttyS1"]); Ok(()) } @@ -354,7 +374,7 @@ match-architectures = ["x86_64", "aarch64"] ostree_commit(repo, &test_rootfs, ".".into(), "testref")?; // Helper closure to read the kargs - let get_kargs = |sys_arch: &str| -> Result> { + let get_kargs = |sys_arch: &str| -> Result { let rootfs = repo.read_commit("testref", cancellable)?.0; let rootfs = rootfs.downcast_ref::().unwrap(); let fetched_tree = rootfs.resolve_relative_path("/usr/lib/bootc/kargs.d"); @@ -368,14 +388,14 @@ match-architectures = ["x86_64", "aarch64"] }; // rootfs is empty - assert_eq!(get_kargs("x86_64").unwrap().len(), 0); + assert_eq!(get_kargs("x86_64").unwrap().iter().count(), 0); test_rootfs.create_dir_all("usr/lib/bootc/kargs.d")?; write_test_kargs(&test_rootfs).unwrap(); ostree_commit(repo, &test_rootfs, ".".into(), "testref")?; let args = get_kargs("x86_64").unwrap(); - similar_asserts::assert_eq!(args, ["console=tty0", "nosmt", "console=ttyS1"]); + assert_cmdline_eq(&args, &["console=tty0", "nosmt", "console=ttyS1"]); Ok(()) } diff --git a/crates/lib/src/deploy.rs b/crates/lib/src/deploy.rs index abc22454d..031700fc1 100644 --- a/crates/lib/src/deploy.rs +++ b/crates/lib/src/deploy.rs @@ -7,6 +7,7 @@ use std::io::{BufRead, Write}; use anyhow::Ok; use anyhow::{anyhow, Context, Result}; +use bootc_kernel_cmdline::utf8::CmdlineOwned; use cap_std::fs::{Dir, MetadataExt}; use cap_std_ext::cap_std; use cap_std_ext::dirext::CapStdExtDirExt; @@ -588,9 +589,9 @@ async fn deploy( let (stateroot, override_kargs) = match &from { MergeState::MergeDeployment(deployment) => { let kargs = crate::bootc_kargs::get_kargs(sysroot, &deployment, image)?; - (deployment.stateroot().into(), kargs) + (deployment.stateroot().into(), Some(kargs)) } - MergeState::Reset { stateroot, kargs } => (stateroot.clone(), kargs.clone()), + MergeState::Reset { stateroot, kargs } => (stateroot.clone(), Some(kargs.clone())), }; // Clone all the things to move to worker thread let ostree = sysroot.get_ostree_cloned()?; @@ -607,13 +608,15 @@ async fn deploy( let stateroot = Some(stateroot); let mut opts = ostree::SysrootDeployTreeOpts::default(); - // Because the C API expects a Vec<&str>, we need to generate a new Vec<> - // that borrows. - let override_kargs = override_kargs - .iter() - .map(|s| s.as_str()) - .collect::>(); - opts.override_kernel_argv = Some(&override_kargs); + // Because the C API expects a Vec<&str>, convert the Cmdline to string slices. + // The references borrow from the Cmdline, which outlives this usage. + let override_kargs_refs = override_kargs + .as_ref() + .map(|kargs| kargs.iter_str().collect::>()); + if let Some(kargs) = override_kargs_refs.as_ref() { + opts.override_kernel_argv = Some(kargs); + } + let deployments = ostree.deployments(); let merge_deployment = merge_deployment.map(|m| &deployments[m]); let origin = glib::KeyFile::new(); @@ -658,7 +661,7 @@ pub(crate) enum MergeState { /// provided initial state. Reset { stateroot: String, - kargs: Vec, + kargs: CmdlineOwned, }, } impl MergeState { diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index 112331819..dec2e0fc3 100644 --- a/crates/lib/src/install.rs +++ b/crates/lib/src/install.rs @@ -27,6 +27,7 @@ use std::time::Duration; use aleph::InstallAleph; use anyhow::{anyhow, ensure, Context, Result}; +use bootc_kernel_cmdline::utf8::{Cmdline, Parameter}; use bootc_utils::CommandRunExt; use camino::Utf8Path; use camino::Utf8PathBuf; @@ -919,7 +920,7 @@ async fn install_container( merged_ostree_root.downcast_ref().unwrap(), std::env::consts::ARCH, )?; - let kargsd = kargsd.iter().map(|s| s.as_str()); + let kargsd = kargsd.iter_str().collect::>(); // If the target uses aboot, then we need to set that bootloader in the ostree // config before deploying the commit @@ -2318,29 +2319,32 @@ pub(crate) async fn install_reset(opts: InstallResetOpts) -> Result<()> { // Compute the kernel arguments to inherit. By default, that's only those involved // in the root filesystem. - let root_kargs = if opts.no_root_kargs { - Vec::new() - } else { + let mut kargs = crate::bootc_kargs::get_kargs_in_root(rootfs, std::env::consts::ARCH)?; + + // Extend with root kargs + if !opts.no_root_kargs { let bootcfg = booted_ostree .deployment .bootconfig() .ok_or_else(|| anyhow!("Missing bootcfg for booted deployment"))?; if let Some(options) = bootcfg.get("options") { - let options = options.split_ascii_whitespace().collect::>(); - crate::bootc_kargs::root_args_from_cmdline(&options) - .into_iter() - .map(ToOwned::to_owned) - .collect::>() - } else { - Vec::new() + let options_cmdline = Cmdline::from(options.as_str()); + let root_kargs = crate::bootc_kargs::root_args_from_cmdline(&options_cmdline); + kargs.extend(&root_kargs); } - }; + } - let kargs = crate::bootc_kargs::get_kargs_in_root(rootfs, std::env::consts::ARCH)? - .into_iter() - .chain(root_kargs.into_iter()) - .chain(opts.karg.unwrap_or_default()) - .collect::>(); + // Extend with user-provided kargs + if let Some(user_kargs) = opts.karg { + let user_kargs = user_kargs + .iter() + .map(|arg| { + Parameter::parse(arg).ok_or_else(|| anyhow!("Unable to parse parameter: {arg}")) + }) + .collect::>>()?; + + kargs.extend(user_kargs); + } let from = MergeState::Reset { stateroot: target_stateroot.clone(), diff --git a/crates/lib/src/install/completion.rs b/crates/lib/src/install/completion.rs index a1b69bd59..32362e354 100644 --- a/crates/lib/src/install/completion.rs +++ b/crates/lib/src/install/completion.rs @@ -67,10 +67,10 @@ fn reconcile_kargs(sysroot: &ostree::Sysroot, deployment: &ostree::Deployment) - .map(|s| s.as_str()) .collect::>(); let kargsd = crate::bootc_kargs::get_kargs_in_root(deployment_root, std::env::consts::ARCH)?; - let kargsd = kargsd.iter().map(|s| s.as_str()).collect::>(); + let kargsd_strs = kargsd.iter_str().collect::>(); current_kargs.append_argv(&install_config_kargs); - current_kargs.append_argv(&kargsd); + current_kargs.append_argv(&kargsd_strs); let new_kargs = current_kargs.to_string(); tracing::debug!("new_kargs={new_kargs}"); diff --git a/tmt/tests/booted/test-image-pushpull-upgrade.nu b/tmt/tests/booted/test-image-pushpull-upgrade.nu index 53b6be3fb..ff1c5abe4 100644 --- a/tmt/tests/booted/test-image-pushpull-upgrade.nu +++ b/tmt/tests/booted/test-image-pushpull-upgrade.nu @@ -10,12 +10,14 @@ use tap.nu const kargsv0 = ["testarg=foo", "othertestkarg", "thirdkarg=bar"] const kargsv1 = ["testarg=foo", "thirdkarg=baz"] let removed = ($kargsv0 | filter { not ($in in $kargsv1) }) +const quoted_karg = '"thisarg=quoted with spaces"' # This code runs on *each* boot. # Here we just capture information. bootc status let st = bootc status --json | from json let booted = $st.status.booted.image +let is_composefs = ($st.status.booted.composefs? != null) # Parse the kernel commandline into a list. # This is not a proper parser, but good enough @@ -76,6 +78,13 @@ RUN echo test content > /usr/share/blah.txt let new_root_mtime = ls -Dl /ostree/bootc | get modified assert ($new_root_mtime > $orig_root_mtime) + # Test for https://github.com/ostreedev/ostree/issues/3544 + # Add a quoted karg using rpm-ostree if available + if not $is_composefs { + print "Adding quoted karg via rpm-ostree to test ostree issue #3544" + rpm-ostree kargs --append=($quoted_karg) + } + # And reboot into it tmt-reboot } @@ -125,6 +134,14 @@ def second_boot [] { assert ($x in $cmdline) } + # Test for https://github.com/ostreedev/ostree/issues/3544 + # Verify the quoted karg added via rpm-ostree is still present + if not $is_composefs { + print "Verifying quoted karg persistence (ostree issue #3544)" + let cmdline = open /proc/cmdline + assert ($quoted_karg in $cmdline) $"Expected quoted karg ($quoted_karg) not found in cmdline" + } + # Now do another build where we drop one of the kargs let td = mktemp -d cd $td @@ -169,6 +186,14 @@ def third_boot [] { assert not ($removed in $cmdline) } + # Test for https://github.com/ostreedev/ostree/issues/3544 + # Verify the quoted karg added via rpm-ostree is still present after upgrade + if not $is_composefs { + print "Verifying quoted karg persistence after upgrade (ostree issue #3544)" + let cmdline = open /proc/cmdline + assert ($quoted_karg in $cmdline) $"Expected quoted karg ($quoted_karg) not found in cmdline after upgrade" + } + tap ok }