Skip to content

efi: Support managing non-EFI components at the ESP root #1073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martinezjavier wants to merge 5 commits into
base: main
Choose a base branch
from
+689 −128
Open

efi: Support managing non-EFI components at the ESP root #1073

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
468f98f
filetree: Decouple source and destination paths in FileTreeDiff
martinezjavier Mar 22, 2026
c8c8811
filetree: Enforce deterministic update ordering in apply_diff()
martinezjavier Mar 22, 2026
0351c03
filetree: Add new_from_dir_strip_prefix_for() for EFILIB layouts
martinezjavier Mar 22, 2026
d750b4e
efi: Operate on the ESP root to support non-EFI components
martinezjavier Mar 22, 2026
b1de2cb
efi: Walk entire ostree-boot/efi/ tree in transfer_ostree_boot_to_usr()
martinezjavier Mar 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 139 additions & 12 deletions src/filetree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ use openssl::hash::{Hasher, MessageDigest};
))]
use rustix::fd::BorrowedFd;
use serde::{Deserialize, Serialize};
#[cfg(any(
target_arch = "x86_64",
target_arch = "aarch64",
target_arch = "riscv64"
))]
use std::cmp::Ordering;
#[allow(unused_imports)]
use std::collections::{BTreeMap, HashMap, HashSet};
use std::fmt::Display;
Expand Down Expand Up @@ -103,6 +109,8 @@ pub(crate) struct FileTreeDiff {
pub(crate) additions: HashSet<String>,
pub(crate) removals: HashSet<String>,
pub(crate) changes: HashSet<String>,
#[serde(default, skip_serializing_if = "HashMap::is_empty")]
pub(crate) source_map: HashMap<String, String>,

@HuijingHei HuijingHei Mar 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to keep only one version instead of multiple? WDYT? @travier @cgwalters

@HuijingHei HuijingHei Mar 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create https://bugzilla.redhat.com/show_bug.cgi?id=2451626 to track the issue: upgrade shim does not remove old version direcory /usr/lib/efi/shim/<older version>

}

impl Display for FileTreeDiff {
Expand Down Expand Up @@ -241,12 +249,18 @@ impl FileTree {
let mut additions = HashSet::new();
let mut removals = HashSet::new();
let mut changes = HashSet::new();
let mut source_map = HashMap::new();

for (k, v1) in self.children.iter() {
if let Some(v2) = updated.children.get(k) {
if v1 != v2 {
// Save the source path for changes
changes.insert(v2.source.as_ref().unwrap_or(k).clone());
// Save the destination key and record the source path for changes
changes.insert(k.clone());
if let Some(src) = v2.source.as_ref() {
if src != k {
source_map.insert(k.clone(), src.clone());
}
}
}
} else {
removals.insert(k.clone());
Expand All @@ -257,14 +271,20 @@ impl FileTree {
if self.children.contains_key(k) {
continue;
}
// Save the source path for additions
additions.insert(v.source.as_ref().unwrap_or(k).clone());
// Save the destination key and record the source path for additions
additions.insert(k.clone());
if let Some(src) = v.source.as_ref() {
if src != k {
source_map.insert(k.clone(), src.clone());
}
}
}
}
Ok(FileTreeDiff {
additions,
removals,
changes,
source_map,
})
}

Expand All @@ -278,6 +298,7 @@ impl FileTree {
pub(crate) fn relative_diff_to(&self, dir: &openat::Dir) -> Result<FileTreeDiff> {
let mut removals = HashSet::new();
let mut changes = HashSet::new();
let mut source_map = HashMap::new();

for (path, info) in self.children.iter() {
assert!(!path.starts_with('/'));
Expand All @@ -288,12 +309,22 @@ impl FileTree {
let target_info = FileMetadata::new_from_path(dir, path)?;
if info != &target_info {
// Save the source path for changes
changes.insert(info.source.as_ref().unwrap_or(path).clone());
changes.insert(path.clone());
if let Some(src) = info.source.as_ref() {
if src != path {
source_map.insert(path.clone(), src.clone());
}
}
}
}
_ => {
// If a file became a directory
changes.insert(info.source.as_ref().unwrap_or(path).clone());
changes.insert(path.clone());
if let Some(src) = info.source.as_ref() {
if src != path {
source_map.insert(path.clone(), src.clone());
}
}
}
}
} else {
Expand All @@ -304,6 +335,7 @@ impl FileTree {
additions: HashSet::new(),
removals,
changes,
source_map,
})
}
}
Expand Down Expand Up @@ -474,9 +506,13 @@ pub(crate) fn apply_diff(
}
// Write changed or new files to temp dir or temp file
for pathstr in diff.changes.iter().chain(diff.additions.iter()) {
let src_path = Utf8Path::new(pathstr);
let path = get_dest_efi_path(src_path);
let (first_dir, first_dir_tmp) = get_first_dir(&path)?;
let path = Utf8Path::new(pathstr);
let copy_src = if let Some(src) = diff.source_map.get(pathstr) {
Utf8Path::new(src)
} else {
path
};
let (first_dir, first_dir_tmp) = get_first_dir(path)?;
let mut path_tmp = Utf8PathBuf::from(&first_dir_tmp);
if first_dir != path {
if !destdir.exists(&first_dir_tmp)? && destdir.exists(first_dir.as_std_path())? {
Expand All @@ -497,13 +533,53 @@ pub(crate) fn apply_diff(
}
updates.insert(first_dir, first_dir_tmp);
srcdir
.copy_file_at(src_path.as_std_path(), destdir, path_tmp.as_std_path())
.with_context(|| format!("copying {:?} to {:?}", src_path, path_tmp))?;
.copy_file_at(copy_src.as_std_path(), destdir, path_tmp.as_std_path())
.with_context(|| format!("copying {:?} to {:?}", copy_src, path_tmp))?;
}

// Sort updates to enforce stable order and atomic sequence
let mut update_list: Vec<_> = updates.iter().collect();
update_list.sort_by(|(dst_a, tmp_a), (dst_b, tmp_b)| {

@HuijingHei HuijingHei Mar 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can make this as separated function?

@Rolv-Apneseth Rolv-Apneseth Apr 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually suggest also pulling all the data required for sorting out of the sort_by loop. The algorithm is (worst-case) O(n * log(n)) and it's best we don't fetch all this data more than once for any entry - especially since there's I/O involved. This would also solve potential issues with keeping the sort deterministic when you have calls to .metadata (which can fail).

Maybe something like this?

struct UpdateEntry {
    dst: Utf8PathBuf,
    tmp: String,
    is_efi: bool,
    is_dir: bool,
}

let mut update_list: Vec<UpdateEntry> = updates
    .into_iter()
    .map(|(dst, tmp)| {
        let is_efi = dst.starts_with("EFI");
        let is_dir = destdir
            .metadata(tmp.as_str())
            .map(|m| m.is_dir())
            .unwrap_or(false);
        UpdateEntry {
            dst,
            tmp,
            is_efi,
            is_dir,
        }
    })
    .collect();

update_list.sort_by(|a, b| {
    b.is_efi
        .cmp(&a.is_efi)
        .then(b.is_dir.cmp(&a.is_dir))
        .then(a.dst.cmp(&b.dst))
});

Potentially another naive question but is it important that root-level directories come before individual files in this ordering?

let a_is_efi = dst_a.starts_with("EFI");
let b_is_efi = dst_b.starts_with("EFI");

// 1. EFI subfolders first
if a_is_efi && !b_is_efi {
return Ordering::Less;
}
if !a_is_efi && b_is_efi {
return Ordering::Greater;
}

// 2. Directories next, then files
let a_is_dir = destdir
.metadata(tmp_a.as_str())
.map(|m| m.is_dir())
.unwrap_or(false);
let b_is_dir = destdir
.metadata(tmp_b.as_str())
.map(|m| m.is_dir())
.unwrap_or(false);

if a_is_dir && !b_is_dir {
return Ordering::Less;
}
if !a_is_dir && b_is_dir {
return Ordering::Greater;
}

// 3. Stable string order
dst_a.cmp(dst_b)
});

// do local exchange or rename
for (dst, tmp) in updates.iter() {
for (dst, tmp) in update_list {
let dst = dst.as_std_path();
if let Some(parent) = dst.parent() {
if !parent.as_os_str().is_empty() {
destdir.ensure_dir_all(parent, DEFAULT_FILE_MODE)?;
}
}
log::trace!("doing local exchange for {} and {:?}", tmp, dst);
if destdir.exists(dst)? {
destdir
Expand Down Expand Up @@ -889,4 +965,55 @@ mod tests {
}
Ok(())
}

/// Test that apply_diff() uses source_map to copy from the original
/// source path when it differs from the destination key.
#[test]
fn test_apply_diff_source_map() -> Result<()> {
let tmpd = tempfile::tempdir()?;
let p = tmpd.path();
let src = p.join("src");
let dst = p.join("dst");
std::fs::create_dir(&src)?;
std::fs::create_dir(&dst)?;

let src_dir = openat::Dir::open(&src)?;
let dst_dir = openat::Dir::open(&dst)?;

// "original/data.bin" -> "remapped/data.bin" via source_map
src_dir.ensure_dir_all("original", 0o755)?;
src_dir.write_file("original/data.bin", 0o644)?;
// Additional files in subdirectories and at the root,
// added without remapping.
src_dir.ensure_dir_all("sub", 0o755)?;
src_dir.write_file("sub/foo.dat", 0o644)?;
src_dir.write_file("top.dat", 0o644)?;

let mut additions = HashSet::new();
additions.insert("remapped/data.bin".to_string());
additions.insert("sub/foo.dat".to_string());
additions.insert("top.dat".to_string());

// source_map: dest path -> source path, only needed when the
// two differ (here a prefix was added to the dest key).
let mut source_map = HashMap::new();
source_map.insert(
"remapped/data.bin".to_string(),
"original/data.bin".to_string(),
);

let diff = FileTreeDiff {
additions,
removals: HashSet::new(),
changes: HashSet::new(),
source_map,
};

apply_diff(&src_dir, &dst_dir, &diff, None)?;

assert!(dst_dir.exists("remapped/data.bin")?);
assert!(dst_dir.exists("sub/foo.dat")?);
assert!(dst_dir.exists("top.dat")?);
Ok(())
}
}