From 53f2d9582921def6c1180a3617baa1f3e4de6123 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 2 Oct 2026 07:34:24 -0400 Subject: [PATCH 1/3] fsck: Write all check results to the given output fsck() takes an output stream, but passing checks were still printed with println!, so a caller capturing the output (or a test) saw only the failures. Also print runtime errors with their context chain, so e.g. "Computing verity state" isn't dropped. Generated-by: AI Signed-off-by: Colin Walters --- crates/lib/src/fsck.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/lib/src/fsck.rs b/crates/lib/src/fsck.rs index 25e95e33cf..aed5fb66fe 100644 --- a/crates/lib/src/fsck.rs +++ b/crates/lib/src/fsck.rs @@ -281,7 +281,7 @@ pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> }; match r { Ok(Ok(())) => { - println!("ok: {name}"); + writeln!(output, "ok: {name}")?; } Ok(Err(e)) => { errors = true; @@ -289,7 +289,7 @@ pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> } Err(e) => { errors = true; - writeln!(output, "Unexpected runtime error in check {name}: {e}")?; + writeln!(output, "Unexpected runtime error in check {name}: {e:#}")?; } } } From ff9eb26d7d5f7cdf30c285db0ff62a654b558d98 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 2 Oct 2026 07:34:31 -0400 Subject: [PATCH 2/3] fsck: Split the resolv.conf check from the host's /usr The check opened the ambient /usr itself, so it could only run on a booted host. Taking the directory makes the check itself testable against a fixture. Prep for converting the checks to return findings. Generated-by: AI Signed-off-by: Colin Walters --- crates/lib/src/fsck.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/lib/src/fsck.rs b/crates/lib/src/fsck.rs index aed5fb66fe..50a8804712 100644 --- a/crates/lib/src/fsck.rs +++ b/crates/lib/src/fsck.rs @@ -114,8 +114,13 @@ fn check_resolvconf(storage: &Storage) -> FsckResult { if ostree.booted_deployment().is_none() { return fsck_ok(); } - // Read usr/etc/resolv.conf directly. let usr = Dir::open_ambient_dir("/usr", cap_std::ambient_authority())?; + check_resolvconf_in(&usr) +} + +/// The resolv.conf check proper, on the `/usr` directory `usr`. +fn check_resolvconf_in(usr: &Dir) -> FsckResult { + // Read usr/etc/resolv.conf directly. let Some(meta) = usr.symlink_metadata_optional("etc/resolv.conf")? else { return fsck_ok(); }; From 3fd1195b288765cbf23939e4f0e5b479cebc1943 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 2 Oct 2026 07:35:16 -0400 Subject: [PATCH 3/3] fsck: Return typed findings from the checks The checks returned at most one free-form error string, so the fsverity check pre-formatted its own truncated list of objects, and nothing could tell the kinds of problems apart. The readonly report being added in https://github.com/bootc-dev/bootc/pull/2497 settled on a two-level shape instead: fatal errors in the outer anyhow::Result, every problem found collected as a typed finding with a subject. Use the same shape here, so the two can share a finding type once that lands. The output stays bounded: the runner prints at most five findings per check and counts the rest, as the fsverity check did itself before. The messages become one "code: subject: detail" line per finding; the tmt tests only look at the exit status. Generated-by: AI Signed-off-by: Colin Walters --- crates/lib/src/fsck.rs | 244 ++++++++++++++++++++++++++++++++--------- 1 file changed, 193 insertions(+), 51 deletions(-) diff --git a/crates/lib/src/fsck.rs b/crates/lib/src/fsck.rs index 50a8804712..5457d126de 100644 --- a/crates/lib/src/fsck.rs +++ b/crates/lib/src/fsck.rs @@ -6,7 +6,6 @@ // Unfortunately needed here to work with linkme #![allow(unsafe_code)] -use std::fmt::Write as _; use std::future::Future; use std::num::NonZeroUsize; use std::pin::Pin; @@ -26,37 +25,55 @@ use crate::store::Storage; use std::os::fd::AsFd; -/// A lint check has failed. -#[derive(thiserror::Error, Debug)] -pub(crate) struct FsckError(String); - -/// The outer error is for unexpected fatal runtime problems; the -/// inner error is for the check failing in an expected way. -pub(crate) type FsckResult = anyhow::Result>; +/// The kind of problem a check found. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum CheckFindingCode { + /// `/usr/etc/resolv.conf` is a zero-sized file. + ResolvconfZeroSized, + /// fsverity is enabled for the ostree repository, but not on an object. + ObjectWithoutFsverity, +} -/// Everything is OK - we didn't encounter a runtime error, and -/// the targeted check passed. -pub(crate) fn fsck_ok() -> FsckResult { - Ok(Ok(())) +impl std::fmt::Display for CheckFindingCode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Debug::fmt(self, f) + } } -/// We successfully found a failure. -pub(crate) fn fsck_err(msg: impl AsRef) -> FsckResult { - Ok(Err(FsckError::new(msg))) +/// A problem a check found in the system. +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct CheckFinding { + code: CheckFindingCode, + /// What the problem is in, e.g. a path or an object. + subject: String, + detail: String, } -impl std::fmt::Display for FsckError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&self.0) +impl CheckFinding { + fn new(code: CheckFindingCode, subject: impl Into, detail: impl Into) -> Self { + Self { + code, + subject: subject.into(), + detail: detail.into(), + } } } -impl FsckError { - fn new(msg: impl AsRef) -> Self { - Self(msg.as_ref().to_owned()) +impl std::fmt::Display for CheckFinding { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}: {}: {}", self.code, self.subject, self.detail) } } +/// Like the lints, checks have two levels of errors: the outer `Result` is +/// for unexpected runtime errors, which stop the check; every problem the +/// check finds in the system is collected as a finding, and the check runs +/// to completion. No findings means the check passed. +pub(crate) type FsckResult = anyhow::Result>; + +/// How many findings of one check are printed; the rest are counted. +const MAX_PRINTED_FINDINGS: NonZeroUsize = NonZeroUsize::new(5).unwrap(); + pub(crate) type FsckFn = fn(&Storage) -> FsckResult; pub(crate) type AsyncFsckFn = fn(&Storage) -> Pin + '_>>; #[derive(Debug)] @@ -108,26 +125,33 @@ static CHECK_RESOLVCONF: FsckCheck = fn check_resolvconf(storage: &Storage) -> FsckResult { let ostree = match storage.get_ostree() { Ok(o) => o, - Err(_) => return fsck_ok(), // Not an ostree system (e.g. composefs-only) + Err(_) => return Ok(Vec::new()), // Not an ostree system (e.g. composefs-only) }; // For now we only check the booted deployment. if ostree.booted_deployment().is_none() { - return fsck_ok(); + return Ok(Vec::new()); } let usr = Dir::open_ambient_dir("/usr", cap_std::ambient_authority())?; check_resolvconf_in(&usr) } +/// The subject of the resolv.conf check's finding. +const RESOLVCONF_SUBJECT: &str = "usr/etc/resolv.conf"; + /// The resolv.conf check proper, on the `/usr` directory `usr`. fn check_resolvconf_in(usr: &Dir) -> FsckResult { // Read usr/etc/resolv.conf directly. let Some(meta) = usr.symlink_metadata_optional("etc/resolv.conf")? else { - return fsck_ok(); + return Ok(Vec::new()); }; if meta.is_file() && meta.size() == 0 { - return fsck_err("Found usr/etc/resolv.conf as zero-sized file"); + return Ok(vec![CheckFinding::new( + CheckFindingCode::ResolvconfZeroSized, + RESOLVCONF_SUBJECT, + "zero-sized file", + )]); } - fsck_ok() + Ok(Vec::new()) } #[derive(Debug, Default)] @@ -242,7 +266,7 @@ fn check_fsverity(storage: &Storage) -> Pin async fn check_fsverity_inner(storage: &Storage) -> FsckResult { let ostree = match storage.get_ostree() { Ok(o) => o, - Err(_) => return fsck_ok(), // Not an ostree system (e.g. composefs-only) + Err(_) => return Ok(Vec::new()), // Not an ostree system (e.g. composefs-only) }; let repo = &ostree.repo(); let verity_state = ostree_ext::fsverity::is_verity_enabled(repo)?; @@ -255,22 +279,44 @@ async fn check_fsverity_inner(storage: &Storage) -> FsckResult { let verity_found_state = verity_state_of_all_objects(&ostree.repo(), verity_state.desired == Tristate::Enabled) .await?; - let Some((missing, rest)) = collect_until( - verity_found_state.missing.iter(), - const { NonZeroUsize::new(5).unwrap() }, - ) else { - return fsck_ok(); + let findings = verity_found_state + .missing + .into_iter() + .map(|obj| { + CheckFinding::new( + CheckFindingCode::ObjectWithoutFsverity, + obj, + "missing fsverity, which is enabled for the repository", + ) + }) + .collect(); + Ok(findings) +} + +/// Print the result of the check `name`, returning whether it passed. +fn print_check_result( + name: &str, + result: &FsckResult, + mut output: impl std::io::Write, +) -> std::io::Result { + let findings = match result { + Ok(findings) => findings, + Err(e) => { + writeln!(output, "Unexpected runtime error in check {name}: {e:#}")?; + return Ok(false); + } }; - let mut err = String::from("fsverity enabled, but objects without fsverity:\n"); - for obj in missing { - // SAFETY: Writing into a String - writeln!(err, " {obj}").unwrap(); + let Some((shown, rest)) = collect_until(findings.iter(), MAX_PRINTED_FINDINGS) else { + writeln!(output, "ok: {name}")?; + return Ok(true); + }; + for finding in shown { + writeln!(output, "fsck error: {name}: {finding}")?; } if rest > 0 { - // SAFETY: Writing into a String - writeln!(err, " ...and {rest} more").unwrap(); + writeln!(output, "fsck error: {name}: ...and {rest} more")?; } - fsck_err(err) + Ok(false) } pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> anyhow::Result<()> { @@ -284,18 +330,8 @@ pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> FsckFnImpl::Sync(f) => f(&storage), FsckFnImpl::Async(f) => f(&storage).await, }; - match r { - Ok(Ok(())) => { - writeln!(output, "ok: {name}")?; - } - Ok(Err(e)) => { - errors = true; - writeln!(output, "fsck error: {name}: {e}")?; - } - Err(e) => { - errors = true; - writeln!(output, "Unexpected runtime error in check {name}: {e:#}")?; - } + if !print_check_result(name, &r, &mut output)? { + errors = true; } } if errors { @@ -316,3 +352,109 @@ pub(crate) async fn fsck(storage: &Storage, mut output: impl std::io::Write) -> Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use cap_std_ext::cap_tempfile; + + #[test] + fn test_check_resolvconf() -> anyhow::Result<()> { + let zero_sized = || { + vec![CheckFinding::new( + CheckFindingCode::ResolvconfZeroSized, + RESOLVCONF_SUBJECT, + "zero-sized file", + )] + }; + // Sets up etc/resolv.conf (if anything) in a fresh /usr + type Setup = fn(&Dir) -> std::io::Result<()>; + let cases: &[(&str, Setup, Vec)] = &[ + ("missing", |_| Ok(()), Vec::new()), + ( + "zero-sized", + |d| d.write("etc/resolv.conf", ""), + zero_sized(), + ), + ( + "nonempty", + |d| d.write("etc/resolv.conf", "nameserver 192.0.2.1\n"), + Vec::new(), + ), + ( + "symlink", + |d| d.symlink("../run/resolv.conf", "etc/resolv.conf"), + Vec::new(), + ), + ("directory", |d| d.create_dir("etc/resolv.conf"), Vec::new()), + ]; + for (name, setup, expected) in cases { + let usr = cap_tempfile::tempdir(cap_std::ambient_authority())?; + usr.create_dir("etc")?; + setup(&usr)?; + assert_eq!(&check_resolvconf_in(&usr)?, expected, "case {name}"); + } + Ok(()) + } + + #[test] + fn test_print_check_result() { + let objects = |n: usize| -> FsckResult { + Ok((0..n) + .map(|i| { + CheckFinding::new( + CheckFindingCode::ObjectWithoutFsverity, + format!("{i:02}.file"), + "detail", + ) + }) + .collect()) + }; + let cases: &[(&str, FsckResult, bool, &str)] = &[ + ("no findings", Ok(Vec::new()), true, "ok: c\n"), + ( + "one finding", + objects(1), + false, + "fsck error: c: ObjectWithoutFsverity: 00.file: detail\n", + ), + ( + "at the limit", + objects(5), + false, + indoc::indoc! {" + fsck error: c: ObjectWithoutFsverity: 00.file: detail + fsck error: c: ObjectWithoutFsverity: 01.file: detail + fsck error: c: ObjectWithoutFsverity: 02.file: detail + fsck error: c: ObjectWithoutFsverity: 03.file: detail + fsck error: c: ObjectWithoutFsverity: 04.file: detail + "}, + ), + ( + "over the limit", + objects(8), + false, + indoc::indoc! {" + fsck error: c: ObjectWithoutFsverity: 00.file: detail + fsck error: c: ObjectWithoutFsverity: 01.file: detail + fsck error: c: ObjectWithoutFsverity: 02.file: detail + fsck error: c: ObjectWithoutFsverity: 03.file: detail + fsck error: c: ObjectWithoutFsverity: 04.file: detail + fsck error: c: ...and 3 more + "}, + ), + ( + "runtime error", + Err(anyhow::anyhow!("inner").context("outer")), + false, + "Unexpected runtime error in check c: outer: inner\n", + ), + ]; + for (name, result, passed, expected) in cases { + let mut output = Vec::new(); + let r = print_check_result("c", result, &mut output).unwrap(); + assert_eq!(r, *passed, "case {name}"); + assert_eq!(String::from_utf8(output).unwrap(), *expected, "case {name}"); + } + } +}