Merge pull request #997 from HuijingHei/improve-step2-prep

Improve `get_efi_vendor()` to use `Path`

Author: HuijingHei

src/bios.rs

@@ -267,7 +267,7 @@ impl Component for Bios {
         Ok(ValidationResult::Skip)
     }
 
-    fn get_efi_vendor(&self, _: &str) -> Result<Option<String>> {
+    fn get_efi_vendor(&self, _: &Path) -> Result<Option<String>> {
         Ok(None)
     }
 }

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);
         }

src/component.rs

@@ -78,7 +78,7 @@ pub(crate) trait Component {
     fn validate(&self, current: &InstalledContent) -> Result<ValidationResult>;
 
     /// Locating efi vendor dir
-    fn get_efi_vendor(&self, sysroot: &str) -> Result<Option<String>>;
+    fn get_efi_vendor(&self, sysroot: &Path) -> Result<Option<String>>;
 }
 
 /// Given a component name, create an implementation.
@@ -204,7 +204,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)?;
@@ -227,13 +226,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)?;
@@ -245,16 +244,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())

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)?
             }
         }
@@ -552,20 +552,15 @@ impl Component for Efi {
         }
     }
 
-    fn get_efi_vendor(&self, sysroot: &str) -> Result<Option<String>> {
-        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<Option<String>> {
+        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