From 072a5f2629b46ba3357acffe2fd2d91bfa746be0 Mon Sep 17 00:00:00 2001 From: Pragyan Poudyal Date: Mon, 19 Jan 2026 14:36:32 +0530 Subject: [PATCH] etc-merge: Create directory in new_etc if deleted If a directory is modified/added in the current etc, but deleted in the new etc, we'd want it in the new etc. This case prior to this commit resulted in a panic as we were not taking it into account Fixes: https://github.com/bootc-dev/bootc/issues/1924 Signed-off-by: Pragyan Poudyal --- crates/etc-merge/src/lib.rs | 74 ++++++++++++++++------ crates/lib/src/bootc_composefs/finalize.rs | 10 +-- crates/lib/src/cli.rs | 10 +-- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/crates/etc-merge/src/lib.rs b/crates/etc-merge/src/lib.rs index 9f687fc6d..ecc4e910b 100644 --- a/crates/etc-merge/src/lib.rs +++ b/crates/etc-merge/src/lib.rs @@ -188,6 +188,7 @@ fn get_deletions( fn get_modifications( pristine: &Directory, current: &Directory, + new: &Directory, mut current_path: PathBuf, diff: &mut Diff, ) -> anyhow::Result<()> { @@ -205,7 +206,21 @@ fn get_modifications( diff.modified.push(current_path.clone()); } - get_modifications(old_dir, &curr_dir, current_path.clone(), diff)? + let total_added = diff.added.len(); + let total_modified = diff.modified.len(); + + get_modifications(old_dir, &curr_dir, new, current_path.clone(), diff)?; + + // This directory or its contents were modified/added + // Check if the new directory was deleted from new_etc + // If it was, we want to add the directory back + if new.get_directory_opt(¤t_path.as_os_str())?.is_none() { + if diff.added.len() != total_added { + diff.added.insert(total_added, current_path.clone()); + } else if diff.modified.len() != total_modified { + diff.modified.insert(total_modified, current_path.clone()); + } + } } Err(ImageError::NotFound(..)) => { @@ -343,6 +358,7 @@ pub fn traverse_etc( pub fn compute_diff( pristine_etc_files: &Directory, current_etc_files: &Directory, + new_etc_files: &Directory, ) -> anyhow::Result { let mut diff = Diff { added: vec![], @@ -353,6 +369,7 @@ pub fn compute_diff( get_modifications( &pristine_etc_files, ¤t_etc_files, + &new_etc_files, PathBuf::new(), &mut diff, )?; @@ -641,7 +658,7 @@ fn merge_leaf( } else { current_etc_fd .copy(&file, new_etc_fd, &file) - .context(format!("Copying file {file:?}"))?; + .with_context(|| format!("Copying file {file:?}"))?; }; rustix::fs::chownat( @@ -719,7 +736,7 @@ pub fn merge( current_etc_dirtree: &Directory, new_etc_fd: &CapStdDir, new_etc_dirtree: &Directory, - diff: Diff, + diff: &Diff, ) -> anyhow::Result<()> { merge_modified_files( &diff.added, @@ -739,7 +756,7 @@ pub fn merge( ) .context("Merging modified files")?; - for removed in diff.removed { + for removed in &diff.removed { let stat = new_etc_fd.metadata_optional(&removed)?; let Some(stat) = stat else { @@ -779,7 +796,7 @@ mod tests { ]; #[test] - fn test_etc_diff() -> anyhow::Result<()> { + fn test_etc_diff_plus_merge() -> anyhow::Result<()> { let tempdir = cap_std_ext::cap_tempfile::tempdir(cap_std::ambient_authority())?; tempdir.create_dir("pristine_etc")?; @@ -822,17 +839,28 @@ mod tests { c.remove_file(deleted_files[0])?; c.remove_file(deleted_files[1])?; - let (pristine_etc_files, current_etc_files, _) = traverse_etc(&p, &c, Some(&n))?; - let res = compute_diff(&pristine_etc_files, ¤t_etc_files)?; + let (pristine_etc_files, current_etc_files, new_etc_files) = + traverse_etc(&p, &c, Some(&n))?; - // Test added files - assert_eq!(res.added.len(), new_files.len()); - assert!(res.added.iter().all(|file| { - new_files - .iter() - .find(|x| PathBuf::from(*x) == *file) - .is_some() - })); + let res = compute_diff( + &pristine_etc_files, + ¤t_etc_files, + new_etc_files.as_ref().unwrap(), + )?; + + merge( + &c, + ¤t_etc_files, + &n, + new_etc_files.as_ref().unwrap(), + &res, + ) + .expect("Merge failed"); + + let added_dirs = ["a", "a/b", "a/b/c"]; + + // 3 for the files, and 3 for the directories + assert_eq!(res.added.len(), new_files.len() + added_dirs.len()); // Test modified files let all_modified_files = overwritten_files @@ -1008,8 +1036,12 @@ mod tests { let (pristine_etc_files, current_etc_files, new_etc_files) = traverse_etc(&p, &c, Some(&n))?; - let diff = compute_diff(&pristine_etc_files, ¤t_etc_files)?; - merge(&c, ¤t_etc_files, &n, &new_etc_files.unwrap(), diff)?; + let diff = compute_diff( + &pristine_etc_files, + ¤t_etc_files, + &new_etc_files.as_ref().unwrap(), + )?; + merge(&c, ¤t_etc_files, &n, &new_etc_files.unwrap(), &diff)?; assert!(files_eq(&c, &n, "new_file.txt")?); assert!(files_eq(&c, &n, "a/new_file.txt")?); @@ -1081,9 +1113,13 @@ mod tests { let (pristine_etc_files, current_etc_files, new_etc_files) = traverse_etc(&p, &c, Some(&n))?; - let diff = compute_diff(&pristine_etc_files, ¤t_etc_files)?; + let diff = compute_diff( + &pristine_etc_files, + ¤t_etc_files, + &new_etc_files.as_ref().unwrap(), + )?; - let merge_res = merge(&c, ¤t_etc_files, &n, &new_etc_files.unwrap(), diff); + let merge_res = merge(&c, ¤t_etc_files, &n, &new_etc_files.unwrap(), &diff); assert!(merge_res.is_err()); assert_eq!( diff --git a/crates/lib/src/bootc_composefs/finalize.rs b/crates/lib/src/bootc_composefs/finalize.rs index 027ffb5ee..feca26f87 100644 --- a/crates/lib/src/bootc_composefs/finalize.rs +++ b/crates/lib/src/bootc_composefs/finalize.rs @@ -11,6 +11,7 @@ use bootc_initramfs_setup::mount_composefs_image; use bootc_mount::tempmount::TempMount; use cap_std_ext::cap_std::{ambient_authority, fs::Dir}; use cap_std_ext::dirext::CapStdExtDirExt; +use composefs::generic_tree::Directory; use etc_merge::{compute_diff, merge, print_diff, traverse_etc}; use rustix::fs::{fsync, renameat}; use rustix::path::Arg; @@ -32,7 +33,7 @@ pub(crate) async fn get_etc_diff(storage: &Storage, booted_cfs: &BootedComposefs let current_etc = Dir::open_ambient_dir("/etc", ambient_authority())?; let (pristine_files, current_files, _) = traverse_etc(&pristine_etc, ¤t_etc, None)?; - let diff = compute_diff(&pristine_files, ¤t_files)?; + let diff = compute_diff(&pristine_files, ¤t_files, &Directory::default())?; print_diff(&diff, &mut std::io::stdout()); @@ -76,10 +77,11 @@ pub(crate) async fn composefs_backend_finalize( let (pristine_files, current_files, new_files) = traverse_etc(&pristine_etc, ¤t_etc, Some(&new_etc))?; - let new_files = new_files.ok_or(anyhow::anyhow!("Failed to get dirtree for new etc"))?; + let new_files = + new_files.ok_or_else(|| anyhow::anyhow!("Failed to get dirtree for new etc"))?; - let diff = compute_diff(&pristine_files, ¤t_files)?; - merge(¤t_etc, ¤t_files, &new_etc, &new_files, diff)?; + let diff = compute_diff(&pristine_files, ¤t_files, &new_files)?; + merge(¤t_etc, ¤t_files, &new_etc, &new_files, &diff)?; // Unmount EROFS drop(erofs_tmp_mnt); diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index b4887f619..567ba8ddc 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -1823,13 +1823,15 @@ async fn run_from_opt(opt: Opt) -> Result<()> { let (p, c, n) = etc_merge::traverse_etc(&pristine_etc, ¤t_etc, Some(&new_etc))?; - let diff = compute_diff(&p, &c)?; + let n = n + .as_ref() + .ok_or_else(|| anyhow::anyhow!("Failed to get new directory tree"))?; + + let diff = compute_diff(&p, &c, &n)?; print_diff(&diff, &mut std::io::stdout()); if merge { - let n = - n.ok_or_else(|| anyhow::anyhow!("Failed to get dirtree for new etc"))?; - etc_merge::merge(¤t_etc, &c, &new_etc, &n, diff)?; + etc_merge::merge(¤t_etc, &c, &new_etc, &n, &diff)?; } Ok(())