From 59f98b0d60fc6a7c5edac44213334b4bb47c4a48 Mon Sep 17 00:00:00 2001 From: Huijing Hei Date: Thu, 4 Sep 2025 16:05:10 +0800 Subject: [PATCH] Improve `get_efi_vendor()` to use `Path` This follows https://github.com/coreos/bootupd/pull/991#discussion_r2320110879 --- src/bios.rs | 2 +- src/bootupd.rs | 2 +- src/component.rs | 16 +++++++--------- src/efi.rs | 21 ++++++++------------- 4 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/bios.rs b/src/bios.rs index 462ba6d2..b4eaca5d 100644 --- a/src/bios.rs +++ b/src/bios.rs @@ -267,7 +267,7 @@ impl Component for Bios { Ok(ValidationResult::Skip) } - fn get_efi_vendor(&self, _: &str) -> Result> { + fn get_efi_vendor(&self, _: &Path) -> Result> { Ok(None) } } diff --git a/src/bootupd.rs b/src/bootupd.rs index 37fc44b6..a8f6affd 100644 --- a/src/bootupd.rs +++ b/src/bootupd.rs @@ -99,7 +99,7 @@ pub(crate) fn install( log::info!("Installed {} {}", component.name(), meta.meta.version); state.installed.insert(component.name().into(), meta); // Yes this is a hack...the Component thing just turns out to be too generic. - if let Some(vendor) = component.get_efi_vendor(&source_root)? { + if let Some(vendor) = component.get_efi_vendor(&Path::new(source_root))? { assert!(installed_efi_vendor.is_none()); installed_efi_vendor = Some(vendor); } diff --git a/src/component.rs b/src/component.rs index c05e6f26..2df768e4 100644 --- a/src/component.rs +++ b/src/component.rs @@ -78,7 +78,7 @@ pub(crate) trait Component { fn validate(&self, current: &InstalledContent) -> Result; /// Locating efi vendor dir - fn get_efi_vendor(&self, sysroot: &str) -> Result>; + fn get_efi_vendor(&self, sysroot: &Path) -> Result>; } /// Given a component name, create an implementation. @@ -202,7 +202,6 @@ mod tests { let tdp = td.path(); let tdupdates = "usr/lib/bootupd/updates/EFI"; let tdir = openat::Dir::open(tdp)?; - let td = tdp.to_str().unwrap(); tdir.ensure_dir_all(tdupdates, 0o755)?; let efi = tdir.sub_dir(tdupdates)?; @@ -225,13 +224,13 @@ mod tests { let target_components: Vec<_> = all_components.values().collect(); for &component in target_components.iter() { if component.name() == "BIOS" { - assert_eq!(component.get_efi_vendor(&td)?, None); + assert_eq!(component.get_efi_vendor(tdp)?, None); } if component.name() == "EFI" { - let x = component.get_efi_vendor(&td); + let x = component.get_efi_vendor(tdp); assert_eq!(x.is_err(), true); efi.remove_all("centos")?; - assert_eq!(component.get_efi_vendor(&td)?, Some("fedora".to_string())); + assert_eq!(component.get_efi_vendor(tdp)?, Some("fedora".to_string())); { let td_vendor = "usr/lib/efi/shim/15.8-3/EFI/centos"; tdir.ensure_dir_all(td_vendor, 0o755)?; @@ -243,16 +242,15 @@ mod tests { )?; // usr/lib/efi wins and get 'centos' - assert_eq!(component.get_efi_vendor(&td)?, Some("centos".to_string())); + assert_eq!(component.get_efi_vendor(tdp)?, Some("centos".to_string())); // find directly from usr/lib/efi and get 'centos' - let td_usr = format!("{td}/usr/lib/efi"); + let td_usr = tdp.join("usr/lib/efi"); assert_eq!( component.get_efi_vendor(&td_usr)?, Some("centos".to_string()) ); // find directly from updates and get 'fedora' - let td_efi = - format!("{td}/{}", component_updatedirname(&**component).display()); + let td_efi = tdp.join(component_updatedirname(&**component)); assert_eq!( component.get_efi_vendor(&td_efi)?, Some("fedora".to_string()) diff --git a/src/efi.rs b/src/efi.rs index 020e1a0b..1286aadf 100644 --- a/src/efi.rs +++ b/src/efi.rs @@ -257,7 +257,7 @@ impl Component for Efi { fn migrate_static_grub_config(&self, sysroot_path: &str, destdir: &openat::Dir) -> Result<()> { let sysroot = openat::Dir::open(sysroot_path).with_context(|| format!("Opening {sysroot_path}"))?; - let Some(vendor) = self.get_efi_vendor(sysroot_path)? else { + let Some(vendor) = self.get_efi_vendor(&Path::new(sysroot_path))? else { anyhow::bail!("Failed to find efi vendor"); }; @@ -380,7 +380,7 @@ impl Component for Efi { .current_dir(format!("/proc/self/fd/{}", src_dir.as_raw_fd())) .run()?; if update_firmware { - if let Some(vendordir) = self.get_efi_vendor(&src_root)? { + if let Some(vendordir) = self.get_efi_vendor(&Path::new(src_root))? { self.update_firmware(device, destd, &vendordir)? } } @@ -545,20 +545,15 @@ impl Component for Efi { } } - fn get_efi_vendor(&self, sysroot: &str) -> Result> { - let sysroot_path = Path::new(sysroot); - let efi_lib = sysroot_path.join(EFILIB); - let updates = sysroot_path.join(component_updatedirname(self)); + fn get_efi_vendor(&self, sysroot: &Path) -> Result> { + let efi_lib = sysroot.join(EFILIB); + let updates = sysroot.join(component_updatedirname(self)); - let target = if let Some(p) = [efi_lib.as_path(), updates.as_path(), sysroot_path] + let paths: [&Path; 3] = [&efi_lib, &updates, sysroot]; + let target = paths .into_iter() .find(|p| p.exists()) - { - p - } else { - bail!("Failed to find valid target path"); - }; - + .ok_or_else(|| anyhow::anyhow!("Failed to find valid target path"))?; let shim_files = find_file_recursive(target, SHIM)?; // Does not support multiple shim for efi