lib/install: Use mounted ESP for install to-filesystem#1953
lib/install: Use mounted ESP for install to-filesystem#1953Guara92 wants to merge 2 commits intobootc-dev:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of the EFI System Partition (ESP) across the codebase. Key changes include introducing a new open_target_root function to abstract opening the target root directory and a require_boot_efi_mount function to validate and retrieve the mounted ESP's source. The require_boot_efi_mount function replaces previous logic for finding and mounting the ESP, ensuring /boot/efi is a valid, mounted vfat/fat filesystem. Consequently, functions like setup_composefs_bls_boot, setup_composefs_uki_boot, install_via_bootupd, install_systemd_boot, and clean_boot_directories are updated to utilize these new helper functions and pass Dir objects for root access. A review comment specifically points out that several parameters (_device, _rootfs, _configopts, _deployment_path) in the install_systemd_boot function are now unused and should be removed for clarity, along with updating their call sites.
| _device: &PartitionTable, | ||
| _rootfs: &Utf8Path, | ||
| _configopts: &crate::install::InstallConfigOpts, | ||
| _deployment_path: Option<&str>, |
There was a problem hiding this comment.
These parameters (_device, _rootfs, _configopts, _deployment_path) are no longer used in this function. To improve code clarity and maintainability, they should be removed from the function signature. The call site in crates/lib/src/bootc_composefs/boot.rs will also need to be updated.
Previously, `bootc install to-filesystem` determined the ESP by scanning the partition table of the disk hosting the target root. This logic failed to respect an explicit `/boot/efi` mount if it was different from the first ESP on the disk. This change updates the installation logic (both for systemd-boot and bootupd paths) to strictly require that `/boot/efi` is mounted under the target root. It inspects this mountpoint to determine the correct backing device, ensuring the installation targets the intended ESP. Assisted-by: Zed Agent (GPT-5.2-Codex) Signed-off-by: Daniele Guarascio <guarascio.daniele@gmail.com>
Guara92
force-pushed
the
fix-esp-selection
branch
from
22641b1 to
4ed589c
Compare
The strict ESP mount enforcement previously introduced caused regressions in scenarios, specifically in CI environments running inside containers (tmt/podman). In these contexts, bind mounts often mask `/boot/efi`, causing `is_mountpoint` checks to fail even when the configuration is valid. This patch introduces a `require_esp_mount` field to `RootSetup`. When targeting an existing root (host), we now utilize a permissive mode: if the explicit mount check fails, logic falls back to scanning the partition table. This restores compatibility with containerized installs while maintaining strict safety checks for `to-filesystem` and `to-disk` modes. Signed-off-by: Daniele Guarascio <guarascio.daniele@gmail.com>
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Thank you so much for your contribution!
Previously, bootc install to-filesystem determined the ESP by scanning the partition table of the disk hosting the target root. This logic failed to respect an explicit /boot/efi mount if it was different from the first ESP on the disk.
Can you explain in what circumstances this happens?
| ) | ||
| } | ||
|
|
||
| fn open_target_root(root_setup: &RootSetup) -> Result<Dir> { |
There was a problem hiding this comment.
Slightly cleaner as a method on impl RootSetup I'd say
| boot, | ||
| kargs, | ||
| skip_finalize: false, | ||
| require_esp_mount: true, |
There was a problem hiding this comment.
Hmm...I would say arguably to-disk should not mount the ESP and we should ensure that it's autodiscovered properly.
| let base_partitions = bootc_blockdev::partitions_of(Utf8Path::new(&device))?; | ||
| let esp = base_partitions.find_partition_of_esp()?; | ||
| Ok(esp.map(|v| v.node.clone())) | ||
| fn normalize_esp_source(source: String) -> String { |
There was a problem hiding this comment.
"normalize" here could use a definition.
Also if we're trying to micro-optimize this it can return a Cow<str>
| return Ok(()); | ||
| }; | ||
| if !esp_fd.is_mountpoint(".")?.unwrap_or(false) { | ||
| anyhow::bail!("/boot/efi is not a mountpoint. This is required for installation."); |
There was a problem hiding this comment.
I don't want to make /boot/efi strictly hard required - among other things the DPS suggests mounting at /efi which makes sense to me and we should handle.
2 participants
Previously,
bootc install to-filesystemdetermined the ESP by scanning the partition table of the disk hosting the target root. This logic failed to respect an explicit/boot/efimount if it was different from the first ESP on the disk.This change updates the installation logic (both for systemd-boot and bootupd paths) to strictly require that
/boot/efiis mounted under the target root. It inspects this mountpoint to determine the correct backing device, ensuring the installation targets the intended ESP.Assisted-by: Zed Agent (GPT-5.2-Codex)
Fixes: #1929