Blockdev cleanup by ckyrouac · Pull Request #2000 · bootc-dev/bootc

ckyrouac · 2026-02-11T16:08:36Z

This was a lot of back and forth with claude. Pushing this for some feedback.

Prep for supporting installing to devices with multiple parents (e.g. RAID, LVM), see #1911. The core change is refactoring bootc-blockdev to use lsblk instead of sfdisk to resolve the device tree. See the individual commits for more details.

gemini-code-assist

Code Review

This pull request refactors the bootc-blockdev crate to use lsblk for device tree resolution instead of sfdisk. This is a significant improvement that prepares the codebase for supporting complex storage layouts like RAID and LVM. The changes include a more robust Device structure with lazy child loading and parent chain traversal. I have provided feedback on the root_disk logic which currently remains restrictive regarding multiple parents, and suggested a minor improvement for sysfs property parsing.

gemini-code-assist · 2026-02-11T16:14:20Z

crates/lib/src/install.rs

+    // udev state is required for running lsblk during install to-disk
+    // see https://github.com/bootc-dev/bootc/pull/688
+    bootc_mount::ensure_mirrored_host_mount("/run/udev")?;


Mounting /run/udev is a critical addition for the reliability of lsblk when running inside a container. This ensures that the container has access to the host's udev database, which is required for lsblk to correctly identify device properties and relationships.

gemini-code-assist · 2026-02-11T16:14:20Z

crates/blockdev/src/blockdev.rs

+            anyhow::ensure!(
+                current.len() == 1,
+                "Device {} has multiple parents; cannot determine root disk",
+                self.path()
+            );


The current implementation of root_disk explicitly bails if a device has multiple parents. While the PR description mentions this is preparation for RAID/LVM support, this specific check will prevent RAID1 or multipath devices from being resolved to a root disk. If the intent is to support these in the future, consider relaxing this or providing a more descriptive error message indicating that RAID/multipath is not yet supported for root disk determination.

cgwalters

Thanks! I think this has the potential for a big cleanup. I have some nits but also some larger structural questions around the end state.

cgwalters · 2026-02-11T20:15:42Z

crates/blockdev/src/blockdev.rs

+            .ok_or_else(|| anyhow!("Missing partition for index {partno}"))
+    }
+
+    /// Find a child partition by partition type GUID.


Note for MBR it's a partition type. I'd just s/ GUID// - or to be more accurate we could explain here that GPT is GUID, MBR is partition type code.

cgwalters · 2026-02-11T20:17:01Z

crates/blockdev/src/blockdev.rs

            first_child.partuuid.as_deref().unwrap(),
            "3979e399-262f-4666-aabc-7ab5d3add2f0"
        );
+        // Verify find_device_by_partno works


This looks sane, but do we have a test JSON that's MBR? Would be good to add here if not

cgwalters · 2026-02-11T20:20:27Z

crates/blockdev/src/blockdev.rs

+    let target = output.blockdevices.first_mut();
+
+    match target {
+        Some(target) => {


Minor style but how about:

let r = match output.blockdevices.as_slice() { [target, ...] => { ...; parents }, [] => Vec::new(); } Ok(r)

cgwalters · 2026-02-11T20:23:36Z

crates/lib/src/bootloader.rs

 // from: https://github.com/systemd/systemd/blob/26b2085d54ebbfca8637362eafcb4a8e3faf832f/man/systemd-boot.xml#L392
 const SYSTEMD_KEY_DIR: &str = "loader/keys";

-#[allow(dead_code)]


👍 nice to see this duplicated code go away

cgwalters · 2026-02-11T20:24:36Z

crates/lib/src/bootloader.rs

+        // Collect partition paths first so they live long enough
+        let partition_paths: Vec<String> =
+            device.children.iter().flatten().map(|p| p.path()).collect();


We could have a Vec<&str> i think if we just moved this call up closer to the function entrypoint

cgwalters · 2026-02-11T20:28:58Z

crates/blockdev/src/blockdev.rs

 }

+/// Find a device in the tree by its path.
+fn find_device_in_tree(root: &Device, target_path: &Utf8Path) -> Option<Device> {


This one looks trivial to generate some unit tests for too

cgwalters · 2026-02-11T20:30:18Z

crates/blockdev/src/blockdev.rs

+    // The "partn" column was added in util-linux 2.39, which is newer than
+    // what CentOS 9 / RHEL 9 ship (2.37).


Ohhhh yes I recall getting burned by a variant of this too 😢

util-linux basically doesn't rebase by default in RHEL so at this point in RHEL9 it's quite old.

But they will patch things if asked, so we could ask...

cgwalters · 2026-02-11T20:31:30Z

crates/blockdev/src/blockdev.rs

+    /// Returns the root device with its children (partitions) populated.
+    /// If this device is already a root device, returns a clone of `self`.
+    /// Fails if the device has multiple parents at any level.
+    pub fn root_disk(&self) -> Result<Device> {


But this function is unused in this commit, which seems surprising?

Probably could also have unit tests.

👍 to unit tests

yea I need to clean up the commits some more. I tried to have claude help but it was a little aggressive on how many commits it created. For reference this function is used in future commits, e.g.

https://github.com/bootc-dev/bootc/pull/2000/changes#diff-1604878878a88c4dcbd7f51444b69fc01a58e3ab48ade4e10b7a9ed05fa867f8R536

https://github.com/bootc-dev/bootc/pull/2000/changes#diff-1604878878a88c4dcbd7f51444b69fc01a58e3ab48ade4e10b7a9ed05fa867f8R1068

cgwalters · 2026-02-11T20:31:57Z

crates/lib/src/bootc_composefs/boot.rs


            // Locate ESP partition device
-            let root_dev = bootc_blockdev::list_dev_by_dir(&storage.physical_root)?;
+            let root_dev = bootc_blockdev::list_dev_by_dir(&storage.physical_root)?.root_disk()?;


(Unrelated to this line of code but I'd argue to squash this with previous)

cgwalters · 2026-02-11T20:34:36Z

crates/lib/src/bootloader.rs

+    let dev = bootc_blockdev::list_dev_by_dir(physical_root)?.root_disk()?;
    if let Some(esp_dev) = dev.find_partition_of_type(bootc_blockdev::ESP) {


But in the end state, don't we want to handle multiple devices? it seems surprising after this series that we end up with calling a fallible .root_disk() in situations like this.

I think this particular case really wants to use a shared find_esp_from_filesystem() or so right? (Also I note here this is using bootc_blockdev::ESP but we need to handle MBR too so we really do need to have a find_esp function which handles either.

root_disk() is sort of a stop-gap function. My current plan for multi device support is to gradually add support depending on the bootloader. So the followup commit to this will add multi device support for uefi and bios installations, but systemd-boot and zipl will continue to only support a single root disk.

cgwalters · 2026-02-11T20:37:26Z

Backing up at a high level what I think would make sense for a series is:

Pure bugfixes/cleanups like the "add missing ptnum for rhel9"
Consolidating a shared "find_esp_from_filesystem" and "find_esp_in_device" functions
Rewriting blockdev.rs (adding new APIs where needed), port things to use the new API?

That said it's possible that the 2. and 3. are too entangled.

ckyrouac · 2026-02-11T20:55:24Z

That said it's possible that the 2. and 3. are too entangled.

They definitely felt entangled as I was iterating on this but I should be able to mostly isolate them.

Add partition and device lookup methods to the Device struct (find_partition_of_type, find_partition_of_esp, find_device_by_partno, refresh, list_parents, root_disk) and list_dev_by_dir for resolving a mounted filesystem's backing device. Migrate all callers from the scattered PartitionTable/Partition-based helpers (esp_in, get_esp_partition, get_sysroot_parent_dev, get_esp_partition_node) to the unified Device API, and change RootSetup.device_info from PartitionTable to Device. Assisted-by: Claude Code (claude-opus-4-5-20251101)

Remove the old partition table types and functions that have been replaced by the unified Device API: - Remove SfDiskOutput, Partition, PartitionType, PartitionTable types - Remove partitions_of() function (use list_dev() + Device methods) - Remove find_parent_devices() function (use Device::list_parents/root_disk) - Remove split_lsblk_line() helper (no longer needed without --pairs) - Remove ESP_ID_MBR constant (MBR ESP support via type codes removed) - Remove regex dependency (was only used by split_lsblk_line) The new Device API provides equivalent functionality: - Device::find_partition_of_type() replaces PartitionTable::find_partition_of_type() - Device::find_partition_of_esp() replaces PartitionTable::find_partition_of_esp() - Device::find_device_by_partno() replaces PartitionTable::find_partno() - Device::root_disk() replaces find_parent_devices() loop pattern Also adds node() as an alias for path() for compatibility, and updates tests to verify the new Device methods. Assisted-by: Claude Code (claude-opus-4-5-20251101) Signed-off-by: ckyrouac <ckyrouac@redhat.com>

ckyrouac · 2026-02-13T21:18:30Z

Updated with cleaned up commits and addressed review comments. Isolating 2 and 3 from #2000 (comment) was getting tricky so they are both in one commit.

github-actions bot added the area/install Issues related to `bootc install` label Feb 11, 2026

bootc-bot bot requested a review from henrywang February 11, 2026 16:09

gemini-code-assist bot reviewed Feb 11, 2026

View reviewed changes

ckyrouac force-pushed the blockdev-cleanup branch from cff3c42 to d399af2 Compare February 11, 2026 16:35

cgwalters requested changes Feb 11, 2026

View reviewed changes

ckyrouac mentioned this pull request Feb 12, 2026

Blockdev bugfixes #2005

Merged

ckyrouac added 2 commits February 13, 2026 13:49

ckyrouac force-pushed the blockdev-cleanup branch from d399af2 to 8435e1d Compare February 13, 2026 21:15

ckyrouac marked this pull request as ready for review February 13, 2026 21:15

ckyrouac changed the title ~~WIP: Blockdev cleanup~~ Blockdev cleanup Feb 13, 2026

		// The "partn" column was added in util-linux 2.39, which is newer than
		// what CentOS 9 / RHEL 9 ship (2.37).

		let dev = bootc_blockdev::list_dev_by_dir(physical_root)?.root_disk()?;
		if let Some(esp_dev) = dev.find_partition_of_type(bootc_blockdev::ESP) {

Conversation

ckyrouac commented Feb 11, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Feb 11, 2026

Uh oh!

ckyrouac commented Feb 11, 2026

Uh oh!

ckyrouac commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants