Uki print dump diff fix

crates/lib/src/bootc_composefs/boot.rs

@@ -64,7 +64,8 @@
 use std::cell::Cell;
 use std::fs::create_dir_all;
 use std::io::{Read, Seek, SeekFrom, Write};
-use std::path::{Path, PathBuf};
+use std::os::fd::AsFd;
+use std::path::Path;
 use std::sync::Arc;
 
 use anyhow::{Context, Result, anyhow, bail};
@@ -77,7 +78,7 @@ use cap_std_ext::{
 use clap::ValueEnum;
 use composefs::fs::read_file;
 use composefs::fsverity::{FsVerityHashValue, Sha512HashValue};
-use composefs::tree::RegularFile;
+use composefs::tree::{FileSystem, RegularFile};
 use composefs_boot::bootloader::{
     BootEntry as ComposefsBootEntry, EFI_ADDON_DIR_EXT, EFI_ADDON_FILE_EXT, EFI_EXT, PEType,
     UsrLibModulesVmlinuz, get_boot_resources,
@@ -102,7 +103,10 @@ use crate::composefs_consts::{TYPE1_BOOT_DIR_PREFIX, TYPE1_ENT_PATH, TYPE1_ENT_P
 use crate::parsers::bls_config::{BLSConfig, BLSConfigType, EFIKey};
 use crate::spec::BootloaderKind;
 use crate::task::Task;
-use crate::{bootc_composefs::repo::open_composefs_repo, store::Storage};
+use crate::{
+    bootc_composefs::repo::open_composefs_repo,
+    store::{ComposefsRepository, Storage},
+};
 use crate::{bootc_composefs::status::get_sorted_grub_uki_boot_entries, install::PostFetchState};
 use crate::{
     composefs_consts::{
@@ -144,6 +148,93 @@ pub(crate) struct UKIDigestMismatch {
     pub uki_name: Option<String>,
 }
 
+pub(crate) fn print_uki_dumpfile_diff(
+    mismatch: &UKIDigestMismatch,
+    repo: &ComposefsRepository,
+    fs: &FileSystem<Sha512HashValue>,
+) {
+    let dumpfile_name = mismatch
+        .uki_name
+        .as_ref()
+        .and_then(|x| x.strip_suffix(EFI_EXT).map(|x| format!("{x}.dump")));
+
+    let Some(dumpfile_name) = &dumpfile_name else {
+        return;
+    };
+
+    let Some(stored_content) = read_dumpfile_from_fs(fs, dumpfile_name, repo) else {
+        tracing::debug!("Dumpfile {dumpfile_name} not found in filesystem");
+        return;
+    };
+
+    let Ok(tempdir) = tempfile::tempdir() else {
+        return;
+    };
+    let path = tempdir.path();
+    let Ok(tempdir_cap) = Dir::open_ambient_dir(path, ambient_authority()) else {
+        return;
+    };
+
+    let Ok(mut stored_file) = tempdir_cap.create("stored") else {
+        return;
+    };
+    if stored_file.write_all(&stored_content).is_err() {
+        return;
+    }
+
+    let Ok(mut current_file) = tempdir_cap.create("current") else {
+        return;
+    };
+    if let Err(e) = dumpfile::write_dumpfile(&mut current_file, fs) {
+        tracing::debug!("Writing dumpfile failed: {e}");
+        return;
+    }
+
+    let mut cmd = std::process::Command::new("diff");
+    cmd.arg("--color=auto")
+        .arg(format!("{}/stored", path.display()))
+        .arg(format!("{}/current", path.display()));
+
+    // Redirect stdout to stderr since this is diagnostic output
+    if let Ok(fd) = std::io::stderr().as_fd().try_clone_to_owned() {
+        cmd.stdout(fd);
+    }
+
+    if let Err(e) = cmd.status() {
+        tracing::warn!("diffing dumpfiles failed with Err: {e:?}");
+    }
+}
+
+fn read_regular_file(
+    file: &RegularFile<Sha512HashValue>,
+    repo: &ComposefsRepository,
+) -> Option<Vec<u8>> {
+    match file {
+        RegularFile::External(object_id, _) | RegularFile::ExternalNoVerity(object_id, _) => {
+            repo.read_object(object_id).ok()
+        }
+        RegularFile::Inline(data) => Some(data.to_vec()),
+        RegularFile::Sparse(_) => None,
+    }
+}
+
+fn read_dumpfile_from_fs(
+    fs: &FileSystem<Sha512HashValue>,
+    dumpfile_name: &str,
+    repo: &ComposefsRepository,
+) -> Option<Vec<u8>> {
+    let root = fs.as_dir();
+    let dumpfile_os = std::ffi::OsStr::new(dumpfile_name);
+
+    if let Ok(boot_dir) = root.get_directory_ref("boot".as_ref()) {
+        if let Ok(file) = boot_dir.get_file(dumpfile_os) {
+            return read_regular_file(file, repo);
+        }
+    }
+
+    None
+}
+
 pub(crate) enum BootSetupType<'a> {
     /// For initial setup, i.e. install to-disk
     Setup((&'a RootSetup, &'a State, &'a PostFetchState)),
@@ -1618,64 +1709,7 @@ pub(crate) async fn setup_composefs_boot(
                 Ok(boot_digest) => boot_digest,
                 Err(e) => match e.downcast::<UKIDigestMismatch>() {
                     Ok(mismatch) => {
-                        // We expect the dumpfile to be named the same as the UKI
-                        // Ex. UKI      - 6.19.14-108.fc42.x86_64.efi
-                        //     Dumpfile - 6.19.14-108.fc42.x86_64.dump
-                        let dumpfile_name = mismatch
-                            .uki_name
-                            .as_ref()
-                            .and_then(|x| x.strip_suffix(EFI_EXT).map(|x| format!("{x}.dump")));
-
-                        let Some(dumpfile_name) = &dumpfile_name else {
-                            return Err(mismatch.into());
-                        };
-
-                        let dump = composefs_ctl::dump_files(
-                            &repo,
-                            &id.to_hex(),
-                            &vec![PathBuf::from(dumpfile_name)],
-                            true,
-                        );
-
-                        let Ok(dump) = dump else {
-                            tracing::debug!("Dumpfile not found for diff");
-                            return Err(mismatch.into());
-                        };
-
-                        // SAFETY: This output is always UTF-8 compatible as it's of the form
-                        // <file-name> <object-path>
-                        let text = std::str::from_utf8(&dump)?;
-                        let obj_path = text.split_whitespace().nth(1);
-
-                        let Some(obj_path) = obj_path else {
-                            return Err(mismatch.into());
-                        };
-
-                        let tempdir = tempfile::tempdir()?;
-                        let path = tempdir.path();
-                        let tempdir = Dir::open_ambient_dir(path, ambient_authority())?;
-
-                        let mut tmpfile = tempdir.create("current")?;
-                        dumpfile::write_dumpfile(&mut tmpfile, &fs).context("Writing dumpfile")?;
-
-                        let mut cmd = std::process::Command::new("diff");
-                        let out = cmd
-                            .arg("--color=auto")
-                            .arg(
-                                root_setup
-                                    .physical_root_path
-                                    .join("sysroot/composefs/objects")
-                                    .join(obj_path),
-                            )
-                            .arg(format!("{}/current", path.display()))
-                            .status();
-
-                        // Intentionally not short-circuiting here as the real error is digest
-                        // mismtach
-                        if let Err(e) = out {
-                            tracing::warn!("diffing dumpfiles failed with Err: {e:?}");
-                        };
-
+                        print_uki_dumpfile_diff(&mismatch, &repo, &fs);
                         return Err(mismatch.into());
                     }
                     Err(e) => Err(e)?,

crates/lib/src/bootc_composefs/repo.rs

@@ -42,6 +42,7 @@ use anyhow::{Context, Result};
 
 use composefs::fsverity::{FsVerityHashValue, Sha512HashValue};
 use composefs::repository::RepositoryConfig;
+use composefs::tree::FileSystem;
 use composefs_boot::bootloader::{BootEntry as ComposefsBootEntry, get_boot_resources};
 use composefs_ctl::composefs;
 use composefs_ctl::composefs_boot;
@@ -188,6 +189,8 @@ pub(crate) struct PullRepoResult {
     pub(crate) id: Sha512HashValue,
     /// The OCI manifest content digest (e.g. "sha256:abc...")
     pub(crate) manifest_digest: String,
+    /// The untransformed OCI filesystem (still has /boot, /sysroot, etc.)
+    pub(crate) fs: FileSystem<Sha512HashValue>,
 }
 
 /// Pull an image directly into the composefs repository via skopeo.
@@ -424,6 +427,7 @@ pub(crate) async fn pull_composefs_repo(
         entries,
         id,
         manifest_digest: pull_result.manifest_digest.to_string(),
+        fs,
     })
 }
 

crates/lib/src/bootc_composefs/update.rs

@@ -17,7 +17,10 @@ use crate::bootc_composefs::gc::GCOpts;
 use crate::spec::BootloaderKind;
 use crate::{
     bootc_composefs::{
-        boot::{BootSetupType, BootType, setup_composefs_bls_boot, setup_composefs_uki_boot},
+        boot::{
+            BootSetupType, BootType, UKIDigestMismatch, print_uki_dumpfile_diff,
+            setup_composefs_bls_boot, setup_composefs_uki_boot,
+        },
         gc::composefs_gc,
         repo::pull_composefs_repo,
         service::start_finalize_stated_svc,
@@ -263,6 +266,7 @@ pub(crate) async fn do_upgrade(
         entries,
         id,
         manifest_digest,
+        fs: oci_fs,
     } = pull_composefs_repo(
         imgref,
         booted_cfs.cmdline.allow_missing_fsverity,
@@ -320,12 +324,25 @@ pub(crate) async fn do_upgrade(
             &mounted_fs,
         )?,
 
-        BootType::Uki => setup_composefs_uki_boot(
-            BootSetupType::Upgrade((storage, booted_cfs, &host)),
-            &repo,
-            &id,
-            entries,
-        )?,
+        BootType::Uki => {
+            let uki_setup_result = setup_composefs_uki_boot(
+                BootSetupType::Upgrade((storage, booted_cfs, &host)),
+                &repo,
+                &id,
+                entries,
+            );
+
+            match uki_setup_result {
+                Ok(boot_digest) => boot_digest,
+                Err(e) => match e.downcast::<UKIDigestMismatch>() {
+                    Ok(mismatch) => {
+                        print_uki_dumpfile_diff(&mismatch, &repo, &oci_fs);
+                        return Err(mismatch.into());
+                    }
+                    Err(e) => Err(e)?,
+                },
+            }
+        }
     };
 
     // `repo` holds its own flock(LOCK_SH) on /sysroot/composefs, taken out by

tmt/plans/integration.fmf

@@ -295,7 +295,7 @@ execute:
       - /tmt/tests/tests/test-46-etc-merge-conflict
 
 /plan-48-composefs-uki-dumpfile:
-  summary: Test composefs garbage collection for UKI
+  summary: Test composefs UKI dumpfile diff print
   discover:
     how: fmf
     test:

tmt/tests/booted/test-composefs-uki-dumpfile.nu

@@ -1,13 +1,14 @@
 # number: 48
 # tmt:
-#   summary: Test composefs garbage collection for UKI
+#   summary: Test composefs UKI dumpfile diff print
 #   duration: 30m
 
 use std assert
 use tap.nu
 
-# FIXME(Johan-Liebert1): This job is disabled for now
-exit 0
+if not (tap is_composefs) {
+    exit 0
+}
 
 # bootc status
 let st = bootc status --json | from json
@@ -37,7 +38,7 @@ def first_boot [] {
 
     let result = do { bootc switch --transport containers-storage localhost/dump-diff } | complete
 
-    let actual_digest = bootc internals cfs oci compute-id $"@(podman images --no-trunc | grep dump-diff | awk '{print $3}')"
+    let actual_digest = bootc internals cfs oci compute-id --bootable $"@(podman images --no-trunc | grep dump-diff | awk '{print $3}')"
 
     assert ($result.exit_code != 0) "bootc switch should fail"
 

tmt/tests/tests.fmf

@@ -185,6 +185,6 @@ check:
   test: nu booted/test-etc-merge-conflict.nu
 
 /test-48-composefs-uki-dumpfile:
-  summary: Test composefs garbage collection for UKI
+  summary: Test composefs UKI dumpfile diff print
   duration: 30m
   test: nu booted/test-composefs-uki-dumpfile.nu