From fb10d63601f8157b79886646031db77e3a86b3ac Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 17 Jan 2025 09:28:58 -0500 Subject: [PATCH 1/3] lints: Add a structure with more info Internal only change, but prep for future work; I want to track the version lints were introduced in, support clearly differentiating between warnings and fatal errors, etc. Specifically motivated by custom base image work https://github.com/coreos/rpm-ostree/issues/5221 but also obviously useful in general. Signed-off-by: Colin Walters --- lib/src/lints.rs | 50 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/lib/src/lints.rs b/lib/src/lints.rs index d382e8e49..b76f2c60d 100644 --- a/lib/src/lints.rs +++ b/lib/src/lints.rs @@ -14,23 +14,51 @@ use fn_error_context::context; /// Reference to embedded default baseimage content that should exist. const BASEIMAGE_REF: &str = "usr/share/doc/bootc/baseimage/base"; +type LintFn = fn(&Dir) -> Result<()>; + +struct Lint { + name: &'static str, + f: LintFn, +} + +const LINTS: &[Lint] = &[ + Lint { + name: "var-run", + f: check_var_run, + }, + Lint { + name: "kernel", + f: check_kernel, + }, + Lint { + name: "bootc-kargs", + f: check_parse_kargs, + }, + Lint { + name: "etc-usretc", + f: check_usretc, + }, + Lint { + name: "utf8", + f: check_utf8, + }, + Lint { + name: "baseimage-root", + f: check_baseimage_root, + }, +]; + /// check for the existence of the /var/run directory /// if it exists we need to check that it links to /run if not error /// if it does not exist error. #[context("Linting")] pub(crate) fn lint(root: &Dir) -> Result<()> { - let lints = [ - check_var_run, - check_kernel, - check_parse_kargs, - check_usretc, - check_utf8, - check_baseimage_root, - ]; - for lint in lints { - lint(&root)?; + for lint in LINTS { + (lint.f)(&root)?; + // We'll be quiet for now + tracing::debug!("OK {}", lint.name); } - println!("Checks passed: {}", lints.len()); + println!("Checks passed: {}", LINTS.len()); Ok(()) } From 09f665a5563b9c4ba98fff95607a1eb57e54d923 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 17 Jan 2025 10:55:53 -0500 Subject: [PATCH 2/3] lint: Add a lint type Everything right now is fatal, but this is prep for adding non-fatal warnings (actually more like the original motivation of lints). Signed-off-by: Colin Walters --- lib/src/lints.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/src/lints.rs b/lib/src/lints.rs index b76f2c60d..f5d8b6263 100644 --- a/lib/src/lints.rs +++ b/lib/src/lints.rs @@ -16,34 +16,50 @@ const BASEIMAGE_REF: &str = "usr/share/doc/bootc/baseimage/base"; type LintFn = fn(&Dir) -> Result<()>; +/// The classification of a lint type. +#[derive(Debug)] +enum LintType { + /// If this fails, it is known to be fatal - the system will not install or + /// is effectively guaranteed to fail at runtime. + Fatal, +} + struct Lint { name: &'static str, + ty: LintType, f: LintFn, } const LINTS: &[Lint] = &[ Lint { name: "var-run", + ty: LintType::Fatal, f: check_var_run, }, Lint { name: "kernel", + ty: LintType::Fatal, f: check_kernel, }, Lint { name: "bootc-kargs", + ty: LintType::Fatal, f: check_parse_kargs, }, Lint { name: "etc-usretc", + ty: LintType::Fatal, f: check_usretc, }, Lint { + // This one can be lifted in the future, see https://github.com/containers/bootc/issues/975 name: "utf8", + ty: LintType::Fatal, f: check_utf8, }, Lint { name: "baseimage-root", + ty: LintType::Fatal, f: check_baseimage_root, }, ]; @@ -56,7 +72,7 @@ pub(crate) fn lint(root: &Dir) -> Result<()> { for lint in LINTS { (lint.f)(&root)?; // We'll be quiet for now - tracing::debug!("OK {}", lint.name); + tracing::debug!("OK {} (type={:?})", lint.name, lint.ty); } println!("Checks passed: {}", LINTS.len()); Ok(()) From 8dd04afc832000546c9e550600e6e973bb9c121d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 17 Jan 2025 11:37:43 -0500 Subject: [PATCH 3/3] lints: Distingiush between "runtime error" vs "lint check" Add an internal error type, and switch to using a nested `Result>` approach for lints. This allows cleanly distinguishing between e.g. "we failed to open a file" from "the lint correctly/successfully found something wrong". This is also helpful for being able to skip lint failures - we wouldn't want to silently skip runtime failures. Signed-off-by: Colin Walters --- Cargo.lock | 13 ++-- lib/Cargo.toml | 1 + lib/src/lints.rs | 164 ++++++++++++++++++++++++++++++----------------- 3 files changed, 112 insertions(+), 66 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7744988cf..f76cc3966 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -196,6 +196,7 @@ dependencies = [ "similar-asserts", "static_assertions", "tempfile", + "thiserror 2.0.11", "tini", "tokio", "tokio-util", @@ -1318,7 +1319,7 @@ dependencies = [ "serde_json", "strum", "strum_macros", - "thiserror 2.0.3", + "thiserror 2.0.11", ] [[package]] @@ -2123,11 +2124,11 @@ dependencies = [ [[package]] name = "thiserror" -version = "2.0.3" +version = "2.0.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c006c85c7651b3cf2ada4584faa36773bd07bac24acfb39f3c431b36d7e667aa" +checksum = "d452f284b73e6d76dd36758a0c8684b1d5be31f92b89d07fd5822175732206fc" dependencies = [ - "thiserror-impl 2.0.3", + "thiserror-impl 2.0.11", ] [[package]] @@ -2143,9 +2144,9 @@ dependencies = [ [[package]] name = "thiserror-impl" -version = "2.0.3" +version = "2.0.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f077553d607adc1caf65430528a576c757a71ed73944b66ebb58ef2bbd243568" +checksum = "26afc1baea8a989337eeb52b6e72a039780ce45c3edfcc9c5b9d112feeb173c2" dependencies = [ "proc-macro2", "quote", diff --git a/lib/Cargo.toml b/lib/Cargo.toml index decdfce87..7d17bdd1a 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -46,6 +46,7 @@ xshell = { version = "0.2.6", optional = true } uuid = { version = "1.8.0", features = ["v4"] } tini = "1.3.0" comfy-table = "7.1.1" +thiserror = "2.0.11" [dev-dependencies] indoc = { workspace = true } diff --git a/lib/src/lints.rs b/lib/src/lints.rs index f5d8b6263..2f0a7acb7 100644 --- a/lib/src/lints.rs +++ b/lib/src/lints.rs @@ -5,7 +5,7 @@ use std::env::consts::ARCH; use std::os::unix::ffi::OsStrExt; -use anyhow::{bail, ensure, Context, Result}; +use anyhow::{Context, Result}; use cap_std::fs::Dir; use cap_std_ext::cap_std; use cap_std_ext::dirext::CapStdExtDirExt as _; @@ -14,7 +14,38 @@ use fn_error_context::context; /// Reference to embedded default baseimage content that should exist. const BASEIMAGE_REF: &str = "usr/share/doc/bootc/baseimage/base"; -type LintFn = fn(&Dir) -> Result<()>; +/// A lint check has failed. +#[derive(thiserror::Error, Debug)] +struct LintError(String); + +/// The outer error is for unexpected fatal runtime problems; the +/// inner error is for the lint failing in an expected way. +type LintResult = Result>; + +/// Everything is OK - we didn't encounter a runtime error, and +/// the targeted check passed. +fn lint_ok() -> LintResult { + Ok(Ok(())) +} + +/// We successfully found a lint failure. +fn lint_err(msg: impl AsRef) -> LintResult { + Ok(Err(LintError::new(msg))) +} + +impl std::fmt::Display for LintError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl LintError { + fn new(msg: impl AsRef) -> Self { + Self(msg.as_ref().to_owned()) + } +} + +type LintFn = fn(&Dir) -> LintResult; /// The classification of a lint type. #[derive(Debug)] @@ -69,104 +100,117 @@ const LINTS: &[Lint] = &[ /// if it does not exist error. #[context("Linting")] pub(crate) fn lint(root: &Dir) -> Result<()> { + let mut fatal = 0usize; + let mut passed = 0usize; for lint in LINTS { - (lint.f)(&root)?; - // We'll be quiet for now - tracing::debug!("OK {} (type={:?})", lint.name, lint.ty); + let name = lint.name; + let r = match (lint.f)(&root) { + Ok(r) => r, + Err(e) => anyhow::bail!("Unexpected runtime error running lint {name}: {e}"), + }; + if let Err(e) = r { + eprintln!("Failed lint: {name}: {e}"); + fatal += 1; + } else { + // We'll be quiet for now + tracing::debug!("OK {name} (type={:?})", lint.ty); + passed += 1; + } + } + println!("Checks passed: {passed}"); + if fatal > 0 { + anyhow::bail!("Checks failed: {fatal}") } - println!("Checks passed: {}", LINTS.len()); Ok(()) } -fn check_var_run(root: &Dir) -> Result<()> { +fn check_var_run(root: &Dir) -> LintResult { if let Some(meta) = root.symlink_metadata_optional("var/run")? { if !meta.is_symlink() { - anyhow::bail!("Not a symlink: var/run"); + return lint_err("Not a symlink: var/run"); } } - Ok(()) + lint_ok() } -fn check_usretc(root: &Dir) -> Result<()> { +fn check_usretc(root: &Dir) -> LintResult { let etc_exists = root.symlink_metadata_optional("etc")?.is_some(); // For compatibility/conservatism don't bomb out if there's no /etc. if !etc_exists { - return Ok(()); + return lint_ok(); } // But having both /etc and /usr/etc is not something we want to support. if root.symlink_metadata_optional("usr/etc")?.is_some() { - anyhow::bail!( + return lint_err( "Found /usr/etc - this is a bootc implementation detail and not supported to use in containers" ); } - Ok(()) + lint_ok() } /// Validate that we can parse the /usr/lib/bootc/kargs.d files. -fn check_parse_kargs(root: &Dir) -> Result<()> { - let _args = crate::kargs::get_kargs_in_root(root, ARCH)?; - Ok(()) +fn check_parse_kargs(root: &Dir) -> LintResult { + let args = crate::kargs::get_kargs_in_root(root, ARCH)?; + tracing::debug!("found kargs: {args:?}"); + lint_ok() } -fn check_kernel(root: &Dir) -> Result<()> { +fn check_kernel(root: &Dir) -> LintResult { let result = ostree_ext::bootabletree::find_kernel_dir_fs(&root)?; tracing::debug!("Found kernel: {:?}", result); - Ok(()) + lint_ok() } -fn check_utf8(dir: &Dir) -> Result<()> { +fn check_utf8(dir: &Dir) -> LintResult { for entry in dir.entries()? { let entry = entry?; let name = entry.file_name(); let Some(strname) = &name.to_str() else { // will escape nicely like "abc\xFFdéf" - bail!("/: Found non-utf8 filename {name:?}"); + return lint_err(format!("/: Found non-utf8 filename {name:?}")); }; let ifmt = entry.file_type()?; if ifmt.is_symlink() { let target = dir.read_link_contents(&name)?; - ensure!( - target.to_str().is_some(), - "/{strname}: Found non-utf8 symlink target" - ); + if !target.to_str().is_some() { + return lint_err(format!("/{strname}: Found non-utf8 symlink target")); + } } else if ifmt.is_dir() { let Some(subdir) = crate::utils::open_dir_noxdev(dir, entry.file_name())? else { continue; }; - if let Err(err) = check_utf8(&subdir) { + if let Err(err) = check_utf8(&subdir)? { // Try to do the path pasting only in the event of an error - bail!("/{strname}{err:?}"); + return lint_err(format!("/{strname}{err}")); } } } - Ok(()) + lint_ok() } /// Check for a few files and directories we expect in the base image. -fn check_baseimage_root_norecurse(dir: &Dir) -> Result<()> { +fn check_baseimage_root_norecurse(dir: &Dir) -> LintResult { // Check /sysroot let meta = dir.symlink_metadata_optional("sysroot")?; match meta { - Some(meta) if !meta.is_dir() => { - anyhow::bail!("Expected a directory for /sysroot") - } - None => anyhow::bail!("Missing /sysroot"), + Some(meta) if !meta.is_dir() => return lint_err("Expected a directory for /sysroot"), + None => return lint_err("Missing /sysroot"), _ => {} } // Check /ostree -> sysroot/ostree let Some(meta) = dir.symlink_metadata_optional("ostree")? else { - anyhow::bail!("Missing ostree -> sysroot/ostree link") + return lint_err("Missing ostree -> sysroot/ostree link"); }; if !meta.is_symlink() { - anyhow::bail!("/ostree should be a symlink"); + return lint_err("/ostree should be a symlink"); } let link = dir.read_link_contents("ostree")?; let expected = "sysroot/ostree"; if link.as_os_str().as_bytes() != expected.as_bytes() { - anyhow::bail!("Expected /ostree -> {expected}, not {link:?}"); + return lint_err("Expected /ostree -> {expected}, not {link:?}"); } // Check the prepare-root config @@ -178,21 +222,21 @@ fn check_baseimage_root_norecurse(dir: &Dir) -> Result<()> { config.load_from_data(&config_data, ostree_ext::glib::KeyFileFlags::empty())?; if !ostree_ext::ostree_prepareroot::overlayfs_enabled_in_config(&config)? { - anyhow::bail!("{prepareroot_path} does not have composefs enabled") + return lint_err("{prepareroot_path} does not have composefs enabled"); } - Ok(()) + lint_ok() } /// Check ostree-related base image content. -fn check_baseimage_root(dir: &Dir) -> Result<()> { - check_baseimage_root_norecurse(dir)?; +fn check_baseimage_root(dir: &Dir) -> LintResult { + check_baseimage_root_norecurse(dir)??; // If we have our own documentation with the expected root contents // embedded, then check that too! Mostly just because recursion is fun. if let Some(dir) = dir.open_dir_optional(BASEIMAGE_REF)? { - check_baseimage_root_norecurse(&dir)?; + check_baseimage_root_norecurse(&dir)??; } - Ok(()) + lint_ok() } #[cfg(test)] @@ -208,12 +252,12 @@ mod tests { fn test_var_run() -> Result<()> { let root = &fixture()?; // This one should pass - check_var_run(root).unwrap(); + check_var_run(root).unwrap().unwrap(); root.create_dir_all("var/run/foo")?; - assert!(check_var_run(root).is_err()); + assert!(check_var_run(root).unwrap().is_err()); root.remove_dir_all("var/run")?; // Now we should pass again - check_var_run(root).unwrap(); + check_var_run(root).unwrap().unwrap(); Ok(()) } @@ -221,7 +265,7 @@ mod tests { fn test_kernel_lint() -> Result<()> { let root = &fixture()?; // This one should pass - check_kernel(root).unwrap(); + check_kernel(root).unwrap().unwrap(); root.create_dir_all("usr/lib/modules/5.7.2")?; root.write("usr/lib/modules/5.7.2/vmlinuz", "old vmlinuz")?; root.create_dir_all("usr/lib/modules/6.3.1")?; @@ -229,14 +273,14 @@ mod tests { assert!(check_kernel(root).is_err()); root.remove_dir_all("usr/lib/modules/5.7.2")?; // Now we should pass again - check_kernel(root).unwrap(); + check_kernel(root).unwrap().unwrap(); Ok(()) } #[test] fn test_kargs() -> Result<()> { let root = &fixture()?; - check_parse_kargs(root).unwrap(); + check_parse_kargs(root).unwrap().unwrap(); root.create_dir_all("usr/lib/bootc")?; root.write("usr/lib/bootc/kargs.d", "not a directory")?; assert!(check_parse_kargs(root).is_err()); @@ -247,13 +291,13 @@ mod tests { fn test_usr_etc() -> Result<()> { let root = &fixture()?; // This one should pass - check_usretc(root).unwrap(); + check_usretc(root).unwrap().unwrap(); root.create_dir_all("etc")?; root.create_dir_all("usr/etc")?; - assert!(check_usretc(root).is_err()); + assert!(check_usretc(root).unwrap().is_err()); root.remove_dir_all("etc")?; // Now we should pass again - check_usretc(root).unwrap(); + check_usretc(root).unwrap().unwrap(); Ok(()) } @@ -274,13 +318,13 @@ mod tests { // Out-of-scope symlinks root.symlink("../../x", "escape").unwrap(); // Should be fine - check_utf8(root).unwrap(); + check_utf8(root).unwrap().unwrap(); // But this will cause an issue let baddir = OsStr::from_bytes(b"subdir/2/bad\xffdir"); root.create_dir("subdir/2").unwrap(); root.create_dir(baddir).unwrap(); - let Err(err) = check_utf8(root) else { + let Err(err) = check_utf8(root).unwrap() else { unreachable!("Didn't fail"); }; assert_eq!( @@ -288,12 +332,12 @@ mod tests { r#"/subdir/2/: Found non-utf8 filename "bad\xFFdir""# ); root.remove_dir(baddir).unwrap(); // Get rid of the problem - check_utf8(root).unwrap(); // Check it + check_utf8(root).unwrap().unwrap(); // Check it // Create a new problem in the form of a regular file let badfile = OsStr::from_bytes(b"regular\xff"); root.write(badfile, b"Hello, world!\n").unwrap(); - let Err(err) = check_utf8(root) else { + let Err(err) = check_utf8(root).unwrap() else { unreachable!("Didn't fail"); }; assert_eq!( @@ -301,11 +345,11 @@ mod tests { r#"/: Found non-utf8 filename "regular\xFF""# ); root.remove_file(badfile).unwrap(); // Get rid of the problem - check_utf8(root).unwrap(); // Check it + check_utf8(root).unwrap().unwrap(); // Check it // And now test invalid symlink targets root.symlink(badfile, "subdir/good-name").unwrap(); - let Err(err) = check_utf8(root) else { + let Err(err) = check_utf8(root).unwrap() else { unreachable!("Didn't fail"); }; assert_eq!( @@ -313,12 +357,12 @@ mod tests { r#"/subdir/good-name: Found non-utf8 symlink target"# ); root.remove_file("subdir/good-name").unwrap(); // Get rid of the problem - check_utf8(root).unwrap(); // Check it + check_utf8(root).unwrap().unwrap(); // Check it // Finally, test a self-referential symlink with an invalid name. // We should spot the invalid name before we check the target. root.symlink(badfile, badfile).unwrap(); - let Err(err) = check_utf8(root) else { + let Err(err) = check_utf8(root).unwrap() else { unreachable!("Didn't fail"); }; assert_eq!( @@ -326,7 +370,7 @@ mod tests { r#"/: Found non-utf8 filename "regular\xFF""# ); root.remove_file(badfile).unwrap(); // Get rid of the problem - check_utf8(root).unwrap(); // Check it + check_utf8(root).unwrap().unwrap(); // Check it } #[test] @@ -358,7 +402,7 @@ mod tests { .arg(".") .run()?; } - check_baseimage_root(&td).unwrap(); + check_baseimage_root(&td).unwrap().unwrap(); Ok(()) } }