blockdev: Clean up ESP discovery code

cgwalters · jmarrero · commit 11ddd188dc7c · 2026-04-06T19:22:28.000-04:00
Several improvements to ESP partition discovery:

Add find_partition_of_esp_optional() returning Result&lt;Option&lt;&amp;Device&gt;&gt;
to cleanly separate three outcomes: found, absent, and genuinely
unexpected errors (like unsupported partition table types). The existing
find_partition_of_esp() is now a thin wrapper that converts None to Err.

Add find_first_colocated_esp() helper to replace a 10-line pattern
that was repeated verbatim 5 times across boot.rs and store/mod.rs.

Deduplicate roots in find_all_roots() using a seen-set: in complex
topologies like multipath, multiple parent branches can converge on the
same physical disk. find_colocated_esps() now uses the optional variant
to properly propagate real errors while treating absence normally.

Also extract the match-on-if-else in setup_composefs_bls_boot into a
let binding for readability.

Assisted-by: OpenCode (Claude Opus 4)
Signed-off-by: Colin Walters &lt;walters@verbum.org&gt;
Signed-off-by: Chris Kyrouac &lt;ckyrouac@redhat.com&gt;
diff --git a/crates/blockdev/src/blockdev.rs b/crates/blockdev/src/blockdev.rs
@@ -1,3 +1,4 @@
+use std::collections::HashSet;
 use std::env;
 use std::path::Path;
 use std::process::{Command, Stdio};
@@ -123,15 +124,26 @@ impl Device {
     /// Calls find_all_roots() to discover physical disks, then searches each for an ESP.
     /// Returns None if no ESPs are found.
     pub fn find_colocated_esps(&self) -> Result<Option<Vec<Device>>> {
-        let esps: Vec<_> = self
-            .find_all_roots()?
-            .iter()
-            .flat_map(|root| root.find_partition_of_esp().ok())
-            .cloned()
-            .collect();
+        let mut esps = Vec::new();
+        for root in &self.find_all_roots()? {
+            if let Some(esp) = root.find_partition_of_esp_optional()? {
+                esps.push(esp.clone());
+            }
+        }
         Ok((!esps.is_empty()).then_some(esps))
     }
 
+    /// Find a single ESP partition among all root devices backing this device.
+    ///
+    /// Walks the parent chain to find all backing disks, then looks for ESP
+    /// partitions on each. Returns the first ESP found. This is the common
+    /// case for composefs/UKI boot paths where exactly one ESP is expected.
+    pub fn find_first_colocated_esp(&self) -> Result<Device> {
+        self.find_colocated_esps()?
+            .and_then(|mut v| Some(v.remove(0)))
+            .ok_or_else(|| anyhow!("No ESP partition found among backing devices"))
+    }
+
     /// Find all BIOS boot partitions across all root devices backing this device.
     /// Calls find_all_roots() to discover physical disks, then searches each for a BIOS boot partition.
     /// Returns None if no BIOS boot partitions are found.
@@ -159,34 +171,41 @@ impl Device {
     ///
     /// For GPT disks, this matches by the ESP partition type GUID.
     /// For MBR (dos) disks, this matches by the MBR partition type IDs (0x06 or 0xEF).
-    pub fn find_partition_of_esp(&self) -> Result<&Device> {
-        let children = self
-            .children
-            .as_ref()
-            .ok_or_else(|| anyhow!("Device has no children"))?;
+    ///
+    /// Returns `Ok(None)` when there are no children or no ESP partition
+    /// is present. Returns `Err` only for genuinely unexpected conditions
+    /// (e.g. an unsupported partition table type).
+    pub fn find_partition_of_esp_optional(&self) -> Result<Option<&Device>> {
+        let Some(children) = self.children.as_ref() else {
+            return Ok(None);
+        };
         match self.pttype.as_deref() {
-            Some("dos") => children
-                .iter()
-                .find(|child| {
-                    child
-                        .parttype
-                        .as_ref()
-                        .and_then(|pt| {
-                            let pt = pt.strip_prefix("0x").unwrap_or(pt);
-                            u8::from_str_radix(pt, 16).ok()
-                        })
-                        .is_some_and(|pt| ESP_ID_MBR.contains(&pt))
-                })
-                .ok_or_else(|| anyhow!("ESP not found in MBR partition table")),
+            Some("dos") => Ok(children.iter().find(|child| {
+                child
+                    .parttype
+                    .as_ref()
+                    .and_then(|pt| {
+                        let pt = pt.strip_prefix("0x").unwrap_or(pt);
+                        u8::from_str_radix(pt, 16).ok()
+                    })
+                    .is_some_and(|pt| ESP_ID_MBR.contains(&pt))
+            })),
             // When pttype is None (e.g. older lsblk or partition devices), default
             // to GPT UUID matching which will simply not match MBR hex types.
-            Some("gpt") | None => self
-                .find_partition_of_type(ESP)
-                .ok_or_else(|| anyhow!("ESP not found in GPT partition table")),
+            Some("gpt") | None => Ok(self.find_partition_of_type(ESP)),
             Some(other) => Err(anyhow!("Unsupported partition table type: {other}")),
         }
     }
 
+    /// Find the EFI System Partition (ESP) among children, or error if absent.
+    ///
+    /// This is a convenience wrapper around [`Self::find_partition_of_esp_optional`]
+    /// for callers that require an ESP to be present.
+    pub fn find_partition_of_esp(&self) -> Result<&Device> {
+        self.find_partition_of_esp_optional()?
+            .ok_or_else(|| anyhow!("ESP partition not found on {}", self.path()))
+    }
+
     /// Find a child partition by partition number (1-indexed).
     pub fn find_device_by_partno(&self, partno: u32) -> Result<&Device> {
         self.children
@@ -308,15 +327,21 @@ impl Device {
         };
 
         let mut roots = Vec::new();
+        let mut seen = HashSet::new();
         let mut queue = parents;
         while let Some(mut device) = queue.pop() {
             match device.children.take() {
                 Some(grandparents) if !grandparents.is_empty() => {
                     queue.extend(grandparents);
                 }
                 _ => {
-                    // Found a root; re-query to populate its actual children
-                    roots.push(list_dev(Utf8Path::new(&device.path()))?);
+                    // Deduplicate: in complex topologies (e.g. multipath)
+                    // multiple branches can converge on the same physical disk.
+                    let name = device.name.clone();
+                    if seen.insert(name) {
+                        // Found a new root; re-query to populate its actual children
+                        roots.push(list_dev(Utf8Path::new(&device.path()))?);
+                    }
                 }
             }
         }
diff --git a/crates/lib/src/bootc_composefs/boot.rs b/crates/lib/src/bootc_composefs/boot.rs
@@ -549,17 +549,7 @@ pub(crate) fn setup_composefs_bls_boot(
             }
 
             // Locate ESP partition device by walking up to the root disk(s)
-            let esp_part = root_setup
-                .device_info
-                .find_colocated_esps()?
-                .and_then(|mut v| {
-                    if v.is_empty() {
-                        None
-                    } else {
-                        Some(v.remove(0))
-                    }
-                })
-                .ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
+            let esp_part = root_setup.device_info.find_first_colocated_esp()?;
 
             (
                 root_setup.physical_root_path.clone(),
@@ -598,16 +588,7 @@ pub(crate) fn setup_composefs_bls_boot(
 
             // Locate ESP partition device by walking up to the root disk(s)
             let root_dev = bootc_blockdev::list_dev_by_dir(&storage.physical_root)?;
-            let esp_dev = root_dev
-                .find_colocated_esps()?
-                .and_then(|mut v| {
-                    if v.is_empty() {
-                        None
-                    } else {
-                        Some(v.remove(0))
-                    }
-                })
-                .ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
+            let esp_dev = root_dev.find_first_colocated_esp()?;
 
             (
                 Utf8PathBuf::from("/sysroot"),
@@ -1116,17 +1097,7 @@ pub(crate) fn setup_composefs_uki_boot(
             state.require_no_kargs_for_uki()?;
 
             // Locate ESP partition device by walking up to the root disk(s)
-            let esp_part = root_setup
-                .device_info
-                .find_colocated_esps()?
-                .and_then(|mut v| {
-                    if v.is_empty() {
-                        None
-                    } else {
-                        Some(v.remove(0))
-                    }
-                })
-                .ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
+            let esp_part = root_setup.device_info.find_first_colocated_esp()?;
 
             (
                 root_setup.physical_root_path.clone(),
@@ -1143,16 +1114,7 @@ pub(crate) fn setup_composefs_uki_boot(
 
             // Locate ESP partition device by walking up to the root disk(s)
             let root_dev = bootc_blockdev::list_dev_by_dir(&storage.physical_root)?;
-            let esp_dev = root_dev
-                .find_colocated_esps()?
-                .and_then(|mut v| {
-                    if v.is_empty() {
-                        None
-                    } else {
-                        Some(v.remove(0))
-                    }
-                })
-                .ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
+            let esp_dev = root_dev.find_first_colocated_esp()?;
 
             (
                 sysroot,
diff --git a/crates/lib/src/bootloader.rs b/crates/lib/src/bootloader.rs
@@ -1,7 +1,7 @@
 use std::fs::create_dir_all;
 use std::process::Command;
 
-use anyhow::{anyhow, bail, Context, Result};
+use anyhow::{Context, Result, anyhow, bail};
 use bootc_utils::{BwrapCmd, CommandRunExt};
 use camino::Utf8Path;
 use cap_std_ext::cap_std::fs::Dir;
@@ -10,8 +10,8 @@ use fn_error_context::context;
 
 use bootc_mount as mount;
 
-use crate::bootc_composefs::boot::{mount_esp, SecurebootKeys};
-use crate::{discoverable_partition_specification, utils};
+use crate::bootc_composefs::boot::{SecurebootKeys, mount_esp};
+use crate::utils;
 
 /// The name of the mountpoint for efi (as a subdirectory of /boot, or at the toplevel)
 pub(crate) const EFI_DIR: &str = "efi";
@@ -53,13 +53,17 @@ pub(crate) fn mount_esp_part(root: &Dir, root_path: &Utf8Path, is_ostree: bool)
 
     let roots = bootc_blockdev::list_dev_by_dir(physical_root)?.find_all_roots()?;
     for dev in &roots {
-        if let Some(esp_dev) = dev.find_partition_of_type(bootc_blockdev::ESP) {
+        if let Some(esp_dev) = dev.find_partition_of_esp_optional()? {
             let esp_path = esp_dev.path();
             bootc_mount::mount(&esp_path, &root_path.join(&efi_path))?;
             tracing::debug!("Mounted {esp_path} at /boot/efi");
             return Ok(());
         }
     }
+    tracing::debug!(
+        "No ESP partition found among {} root device(s)",
+        roots.len()
+    );
     Ok(())
 }
 
@@ -228,10 +232,14 @@ pub(crate) fn install_systemd_boot(
     autoenroll: Option<SecurebootKeys>,
 ) -> Result<()> {
     let roots = device.find_all_roots()?;
-    let esp_part = roots
-        .iter()
-        .find_map(|root| root.find_partition_of_type(discoverable_partition_specification::ESP))
-        .ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
+    let mut esp_part = None;
+    for root in &roots {
+        if let Some(esp) = root.find_partition_of_esp_optional()? {
+            esp_part = Some(esp);
+            break;
+        }
+    }
+    let esp_part = esp_part.ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
 
     let esp_mount = mount_esp(&esp_part.path()).context("Mounting ESP")?;
     let esp_path = Utf8Path::from_path(esp_mount.dir.path())
diff --git a/crates/lib/src/store/mod.rs b/crates/lib/src/store/mod.rs
@@ -200,16 +200,7 @@ impl BootedStorage {
 
                 // Locate ESP by walking up to the root disk(s)
                 let root_dev = bootc_blockdev::list_dev_by_dir(&physical_root)?;
-                let esp_dev = root_dev
-                    .find_colocated_esps()?
-                    .and_then(|mut v| {
-                        if v.is_empty() {
-                            None
-                        } else {
-                            Some(v.remove(0))
-                        }
-                    })
-                    .ok_or_else(|| anyhow::anyhow!("ESP partition not found"))?;
+                let esp_dev = root_dev.find_first_colocated_esp()?;
                 let esp_mount = mount_esp(&esp_dev.path())?;
 
                 let boot_dir = match get_bootloader()? {
