From b9ea8b615a73ae1f1ec9ab81714eaa7a874b95bc Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 27 Jun 2024 17:30:30 -0400 Subject: [PATCH 1/4] kargs: Factor out helper function for ostree case Just reducing the size of the main `get_kargs`. Now that we've split out the helper, add a unit test. Signed-off-by: Colin Walters --- lib/src/kargs.rs | 163 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 128 insertions(+), 35 deletions(-) diff --git a/lib/src/kargs.rs b/lib/src/kargs.rs index d4815cbe..864ac419 100644 --- a/lib/src/kargs.rs +++ b/lib/src/kargs.rs @@ -49,6 +49,43 @@ fn get_kargs_in_root(d: &Dir, sys_arch: &str) -> Result> { Ok(ret) } +/// Load kargs.d files from the target ostree commit root +fn get_kargs_from_ostree( + repo: &ostree::Repo, + fetched_tree: &ostree::RepoFile, + sys_arch: &str, +) -> 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(); + 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(); + if let Some(name) = name.to_str() { + if name.ends_with(".toml") { + let fetched_child = fetched_iter.child(&fetched_info); + let fetched_child = fetched_child + .downcast::() + .expect("downcast"); + fetched_child.ensure_resolved()?; + let fetched_contents_checksum = fetched_child.checksum(); + let f = + ostree::Repo::load_file(repo, fetched_contents_checksum.as_str(), cancellable)?; + let file_content = f.0; + 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); + } + } + } + Ok(ret) +} + /// Compute the kernel arguments for the new deployment. This starts from the booted /// karg, but applies the diff between the bootc karg files in /usr/lib/bootc/kargs.d /// between the booted deployment and the new one. @@ -86,33 +123,7 @@ pub(crate) fn get_kargs( return Ok(kargs); } - let mut remote_kargs: Vec = vec![]; - let queryattrs = "standard::name,standard::type"; - let queryflags = gio::FileQueryInfoFlags::NOFOLLOW_SYMLINKS; - let fetched_iter = fetched_tree.enumerate_children(queryattrs, queryflags, cancellable)?; - 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(); - if let Some(name) = name.to_str() { - if name.ends_with(".toml") { - let fetched_child = fetched_iter.child(&fetched_info); - let fetched_child = fetched_child - .downcast::() - .expect("downcast"); - fetched_child.ensure_resolved()?; - let fetched_contents_checksum = fetched_child.checksum(); - let f = - ostree::Repo::load_file(repo, fetched_contents_checksum.as_str(), cancellable)?; - let file_content = f.0; - let mut reader = - ostree_ext::prelude::InputStreamExtManual::into_read(file_content.unwrap()); - let s = std::io::read_to_string(&mut reader)?; - let mut parsed_kargs = - parse_kargs_toml(&s, sys_arch).with_context(|| format!("Parsing {name}"))?; - remote_kargs.append(&mut parsed_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 @@ -156,6 +167,9 @@ fn parse_kargs_toml(contents: &str, sys_arch: &str) -> Result> { #[cfg(test)] mod tests { + use fn_error_context::context; + use rustix::fd::{AsFd, AsRawFd}; + use super::*; #[test] @@ -208,6 +222,20 @@ match-architectures = ["x86_64", "aarch64"] assert!(parse_kargs_toml(test_missing, "x86_64").is_err()); } + #[context("writing test kargs")] + fn write_test_kargs(td: &Dir) -> Result<()> { + td.write( + "usr/lib/bootc/kargs.d/01-foo.toml", + r##"kargs = ["console=tty0", "nosmt"]"##, + )?; + td.write( + "usr/lib/bootc/kargs.d/02-bar.toml", + r##"kargs = ["console=ttyS1"]"##, + )?; + + Ok(()) + } + #[test] fn test_get_kargs_in_root() -> Result<()> { let td = cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?; @@ -220,18 +248,83 @@ match-architectures = ["x86_64", "aarch64"] // Non-toml file td.write("usr/lib/bootc/kargs.d/somegarbage", "garbage")?; assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().len(), 0); - td.write( - "usr/lib/bootc/kargs.d/01-foo.toml", - r##"kargs = ["console=tty0", "nosmt"]"##, - )?; - td.write( - "usr/lib/bootc/kargs.d/02-bar.toml", - r##"kargs = ["console=ttyS1"]"##, - )?; + + write_test_kargs(&td)?; let args = get_kargs_in_root(&td, "x86_64").unwrap(); similar_asserts::assert_eq!(args, ["console=tty0", "nosmt", "console=ttyS1"]); Ok(()) } + + #[context("ostree commit")] + fn ostree_commit( + repo: &ostree::Repo, + d: &Dir, + path: &Utf8Path, + ostree_ref: &str, + ) -> Result<()> { + let cancellable = gio::Cancellable::NONE; + let txn = repo.auto_transaction(cancellable)?; + + let mt = ostree::MutableTree::new(); + repo.write_dfd_to_mtree(d.as_fd().as_raw_fd(), path.as_str(), &mt, None, cancellable) + .context("Writing merged filesystem to mtree")?; + + let merged_root = repo + .write_mtree(&mt, cancellable) + .context("Writing mtree")?; + let merged_root = merged_root.downcast::().unwrap(); + let merged_commit = repo + .write_commit(None, None, None, None, &merged_root, cancellable) + .context("Writing commit")?; + repo.transaction_set_ref(None, &ostree_ref, Some(merged_commit.as_str())); + txn.commit(cancellable)?; + Ok(()) + } + + #[test] + fn test_get_kargs_in_ostree() -> Result<()> { + let cancellable = gio::Cancellable::NONE; + let td = cap_std_ext::cap_tempfile::TempDir::new(cap_std::ambient_authority())?; + + td.create_dir("repo")?; + let repo = &ostree::Repo::create_at( + td.as_fd().as_raw_fd(), + "repo", + ostree::RepoMode::Bare, + None, + gio::Cancellable::NONE, + )?; + + td.create_dir("rootfs")?; + let test_rootfs = &td.open_dir("rootfs")?; + + ostree_commit(repo, &test_rootfs, ".".into(), "testref")?; + // Helper closure to read the kargs + 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"); + let fetched_tree = fetched_tree + .downcast::() + .expect("downcast"); + if !fetched_tree.query_exists(cancellable) { + return Ok(Default::default()); + } + get_kargs_from_ostree(repo, &fetched_tree, sys_arch) + }; + + // rootfs is empty + assert_eq!(get_kargs("x86_64").unwrap().len(), 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"]); + + Ok(()) + } } From e40e235fdeafb1febd27976e7099a709171d2c0e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 27 Jun 2024 20:29:18 -0400 Subject: [PATCH 2/4] kargs: De-indent ostree reading logic Instead of nested `if`, use `continue` to de-indent. Signed-off-by: Colin Walters --- lib/src/kargs.rs | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/lib/src/kargs.rs b/lib/src/kargs.rs index 864ac419..6de0a51d 100644 --- a/lib/src/kargs.rs +++ b/lib/src/kargs.rs @@ -63,25 +63,28 @@ fn get_kargs_from_ostree( 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(); - if let Some(name) = name.to_str() { - if name.ends_with(".toml") { - let fetched_child = fetched_iter.child(&fetched_info); - let fetched_child = fetched_child - .downcast::() - .expect("downcast"); - fetched_child.ensure_resolved()?; - let fetched_contents_checksum = fetched_child.checksum(); - let f = - ostree::Repo::load_file(repo, fetched_contents_checksum.as_str(), cancellable)?; - let file_content = f.0; - 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); - } + let name = if let Some(name) = name.to_str() { + name + } else { + continue; + }; + if !name.ends_with(".toml") { + continue; } + let fetched_child = fetched_iter.child(&fetched_info); + let fetched_child = fetched_child + .downcast::() + .expect("downcast"); + fetched_child.ensure_resolved()?; + let fetched_contents_checksum = fetched_child.checksum(); + let f = ostree::Repo::load_file(repo, fetched_contents_checksum.as_str(), cancellable)?; + let file_content = f.0; + 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); } Ok(ret) } From 6ea18021e74d556c526805a99a2767c2e2d43522 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 27 Jun 2024 21:07:42 -0400 Subject: [PATCH 3/4] kargs: Add a few more comments Signed-off-by: Colin Walters --- lib/src/kargs.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/src/kargs.rs b/lib/src/kargs.rs index 6de0a51d..6d730745 100644 --- a/lib/src/kargs.rs +++ b/lib/src/kargs.rs @@ -13,10 +13,14 @@ use serde::Deserialize; use crate::deploy::ImageState; +/// The kargs.d configuration file. #[derive(Deserialize)] #[serde(rename_all = "kebab-case", deny_unknown_fields)] struct Config { + /// Ordered list of kernel arguments. kargs: Vec, + /// Optional list of architectures (using the Rust naming conventions); + /// if present and the current architecture doesn't match, the file is skipped. match_architectures: Option>, } @@ -126,6 +130,7 @@ pub(crate) fn get_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 From 60006335a939061a6c41164e908b245d144cbd60 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 27 Jun 2024 21:10:59 -0400 Subject: [PATCH 4/4] kargs: Factor out helper for filename matching Since we have two different read paths. Signed-off-by: Colin Walters --- lib/src/kargs.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/src/kargs.rs b/lib/src/kargs.rs index 6d730745..f13d7d43 100644 --- a/lib/src/kargs.rs +++ b/lib/src/kargs.rs @@ -24,6 +24,13 @@ struct Config { match_architectures: Option>, } +impl Config { + /// Return true if the filename is one we should parse. + fn filename_matches(name: &str) -> bool { + matches!(Utf8Path::new(name).extension(), Some("toml")) + } +} + /// Load and parse all bootc kargs.d files in the specified root, returning /// a combined list. fn get_kargs_in_root(d: &Dir, sys_arch: &str) -> Result> { @@ -43,7 +50,7 @@ fn get_kargs_in_root(d: &Dir, sys_arch: &str) -> Result> { let name = name .to_str() .ok_or_else(|| anyhow::anyhow!("Invalid non-UTF8 filename: {name:?}"))?; - if !matches!(Utf8Path::new(name).extension(), Some("toml")) { + if !Config::filename_matches(name) { continue; } let buf = d.read_to_string(name)?; @@ -72,9 +79,10 @@ fn get_kargs_from_ostree( } else { continue; }; - if !name.ends_with(".toml") { + if !Config::filename_matches(name) { continue; } + let fetched_child = fetched_iter.child(&fetched_info); let fetched_child = fetched_child .downcast::()