Merge pull request #645 from cgwalters/kargs-cleanup

kargs: Various cleanups

Author: cgwalters

Cargo.lock

@@ -25,7 +25,7 @@ RUN /tmp/provision-derived.sh "$variant" && rm -f /tmp/*.sh
 COPY hack/install-test-configs/* /usr/lib/bootc/install/
 # Inject our built code
 COPY --from=build /out/bootc.tar.zst /tmp
-RUN tar -C / --zstd -xvf /tmp/bootc.tar.zst && rm -vf /tmp/*
+RUN tar -C / --zstd -xvf /tmp/bootc.tar.zst && rm -vrf /tmp/*
 # Also copy over arbitrary bits from the target root
 COPY --from=build /build/target/dev-rootfs/ /
 # Test our own linting

hack/Containerfile

@@ -46,6 +46,9 @@ toml = "0.8.12"
 xshell = { version = "0.2.6", optional = true }
 uuid = { version = "1.8.0", features = ["v4"] }
 
+[dev-dependencies]
+similar-asserts = { version = "1.5.0" }
+
 [features]
 default = ["install"]
 # This feature enables `bootc install`.  Disable if you always want to use an external installer.

lib/Cargo.toml

@@ -1,5 +1,8 @@
-use anyhow::Ok;
-use anyhow::Result;
+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;
 use ostree_ext::ostree;
 use ostree_ext::ostree::Deployment;
@@ -17,6 +20,35 @@ struct Config {
     match_architectures: Option<Vec<String>>,
 }
 
+/// 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<Vec<String>> {
+    // If the directory doesn't exist, that's OK.
+    let d = if let Some(d) = d.open_dir_optional("usr/lib/bootc/kargs.d")? {
+        d
+    } else {
+        return Ok(Default::default());
+    };
+    let mut ret = Vec::new();
+    // Read all the entries
+    let mut entries = d.entries()?.collect::<std::io::Result<Vec<_>>>()?;
+    // cc https://github.com/rust-lang/rust/issues/85573 re the allocation-per-comparison here
+    entries.sort_by(|a, b| a.file_name().cmp(&b.file_name()));
+    for ent in entries {
+        let name = ent.file_name();
+        let name = name
+            .to_str()
+            .ok_or_else(|| anyhow::anyhow!("Invalid non-UTF8 filename: {name:?}"))?;
+        if !matches!(Utf8Path::new(name).extension(), Some("toml")) {
+            continue;
+        }
+        let buf = d.read_to_string(name)?;
+        let kargs = parse_kargs_toml(&buf, sys_arch).with_context(|| format!("Parsing {name}"))?;
+        ret.extend(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.
@@ -27,38 +59,34 @@ pub(crate) fn get_kargs(
 ) -> Result<Vec<String>> {
     let cancellable = gio::Cancellable::NONE;
     let mut kargs: Vec<String> = vec![];
-    let sys_arch = std::env::consts::ARCH.to_string();
+    let sys_arch = std::env::consts::ARCH;
 
     // Get the running kargs of the booted system
     if let Some(bootconfig) = ostree::Deployment::bootconfig(booted_deployment) {
         if let Some(options) = ostree::BootconfigParser::get(&bootconfig, "options") {
-            let options: Vec<&str> = options.split_whitespace().collect();
-            let mut options: Vec<String> = options.into_iter().map(|s| s.to_string()).collect();
-            kargs.append(&mut 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 mut existing_kargs: Vec<String> = vec![];
-    let fragments = liboverdrop::scan(&["/usr/lib"], "bootc/kargs.d", &["toml"], true);
-    for (_name, path) in fragments {
-        let s = std::fs::read_to_string(&path)?;
-        let mut parsed_kargs = parse_file(s.clone(), sys_arch.clone())?;
-        existing_kargs.append(&mut parsed_kargs);
-    }
+    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 pending image
-    let mut remote_kargs: Vec<String> = vec![];
     let (fetched_tree, _) = repo.read_commit(fetched.ostree_commit.as_str(), cancellable)?;
     let fetched_tree = fetched_tree.resolve_relative_path("/usr/lib/bootc/kargs.d");
     let fetched_tree = fetched_tree
         .downcast::<ostree::RepoFile>()
         .expect("downcast");
+    // 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) {
-        // if the kargs.d directory does not exist in the fetched image, return the existing kargs
-        kargs.append(&mut existing_kargs);
+        kargs.extend(existing_kargs);
         return Ok(kargs);
     }
+
+    let mut remote_kargs: Vec<String> = vec![];
     let queryattrs = "standard::name,standard::type";
     let queryflags = gio::FileQueryInfoFlags::NOFOLLOW_SYMLINKS;
     let fetched_iter = fetched_tree.enumerate_children(queryattrs, queryflags, cancellable)?;
@@ -79,7 +107,8 @@ pub(crate) fn get_kargs(
                 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_file(s.clone(), sys_arch.clone())?;
+                let mut parsed_kargs =
+                    parse_kargs_toml(&s, sys_arch).with_context(|| format!("Parsing {name}"))?;
                 remote_kargs.append(&mut parsed_kargs);
             }
         }
@@ -110,58 +139,99 @@ pub(crate) fn get_kargs(
     Ok(kargs)
 }
 
-pub fn parse_file(file_content: String, sys_arch: String) -> Result<Vec<String>> {
-    let mut de: Config = toml::from_str(&file_content)?;
-    let mut parsed_kargs: Vec<String> = vec![];
+/// 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<Vec<String>> {
+    let de: Config = toml::from_str(contents)?;
     // if arch specified, apply kargs only if the arch matches
     // if arch not specified, apply kargs unconditionally
-    match de.match_architectures {
-        None => parsed_kargs = de.kargs,
-        Some(match_architectures) => {
-            if match_architectures.contains(&sys_arch) {
-                parsed_kargs.append(&mut de.kargs);
-            }
-        }
-    }
-    Ok(parsed_kargs)
+    let matched = de
+        .match_architectures
+        .map(|arches| arches.iter().any(|s| s == sys_arch))
+        .unwrap_or(true);
+    let r = if matched { de.kargs } else { Vec::new() };
+    Ok(r)
 }
 
-#[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".to_string();
-    let file_content = r##"kargs = ["console=tty0", "nosmt"]"##.to_string();
-    let parsed_kargs = parse_file(file_content.clone(), sys_arch.clone()).unwrap();
-    assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
-    let sys_arch = "aarch64".to_string();
-    let parsed_kargs = parse_file(file_content.clone(), sys_arch.clone()).unwrap();
-    assert_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".to_string();
-    let file_content = r##"kargs = ["console=tty0", "nosmt"]
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[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 sys_arch = "aarch64";
+        let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap();
+        assert_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";
+        let file_content = r##"kargs = ["console=tty0", "nosmt"]
 match-architectures = ["x86_64"]
 "##
-    .to_string();
-    let parsed_kargs = parse_file(file_content.clone(), sys_arch.clone()).unwrap();
-    assert_eq!(parsed_kargs, [] as [String; 0]);
-    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, [] as [String; 0]);
+        let file_content = r##"kargs = ["console=tty0", "nosmt"]
 match-architectures = ["aarch64"]
 "##
-    .to_string();
-    let parsed_kargs = parse_file(file_content.clone(), sys_arch.clone()).unwrap();
-    assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
+        .to_string();
+        let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap();
+        assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
 
-    // multiple arch specified, ensure that kargs are applied to both archs
-    let sys_arch = "x86_64".to_string();
-    let file_content = r##"kargs = ["console=tty0", "nosmt"]
+        // multiple arch specified, ensure that kargs are applied to both archs
+        let sys_arch = "x86_64";
+        let file_content = r##"kargs = ["console=tty0", "nosmt"]
 match-architectures = ["x86_64", "aarch64"]
 "##
-    .to_string();
-    let parsed_kargs = parse_file(file_content.clone(), sys_arch.clone()).unwrap();
-    assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
-    std::env::set_var("ARCH", "aarch64");
-    let parsed_kargs = parse_file(file_content.clone(), sys_arch.clone()).unwrap();
-    assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
+        .to_string();
+        let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap();
+        assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
+        std::env::set_var("ARCH", "aarch64");
+        let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap();
+        assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]);
+    }
+
+    #[test]
+    /// Verify some error cases
+    fn test_invalid() {
+        let test_invalid_extra = r#"kargs = ["console=tty0", "nosmt"]\nfoo=bar"#;
+        assert!(parse_kargs_toml(test_invalid_extra, "x86_64").is_err());
+
+        let test_missing = r#"foo=bar"#;
+        assert!(parse_kargs_toml(test_missing, "x86_64").is_err());
+    }
+
+    #[test]
+    fn test_get_kargs_in_root() -> Result<()> {
+        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);
+        // Empty directory
+        td.create_dir_all("usr/lib/bootc/kargs.d")?;
+        assert_eq!(get_kargs_in_root(&td, "x86_64").unwrap().len(), 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);
+        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"]"##,
+        )?;
+
+        let args = get_kargs_in_root(&td, "x86_64").unwrap();
+        similar_asserts::assert_eq!(args, ["console=tty0", "nosmt", "console=ttyS1"]);
+
+        Ok(())
+    }
 }