cfs/boot: Handle separate /boot mount - #2440
Johan-Liebert1 wants to merge 1 commit into
Conversation
If /boot is mounted as XBOOTLDR partition then grub configs are stored there as we were unconditionally searching for grub configs in `/sysroot/boot`, which would fail every time. Fix this by checking if `/boot` is a mountpoint and then checking if we have grub configs in there. This is only an issue with Grub as GrubCC and SystemdBoot both do not store anything inside of `/sysroot/boot` and will (should) always have the ESP mounted at /boot Closes: bootc-dev#2399 Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Actually won't it be at |
|
Oh, you're right, that's the case with both ESP and XBOOTLDR. So this needs to be updated for the other two bootloaders as well. I'll add that |
|
So this isn't actually an issue for us since we manually search for the ESP and temp mount it to get a handle. Trying the following setup, everything works as intended [root@fedora boot]# findmnt /boot
TARGET SOURCE FSTYPE OPTIONS
/boot systemd-1 autofs rw,relatime,fd=89,pgrp=1,timeout=120,minproto=5,maxproto=5,direct,pipe_ino=9419
/boot /dev/vda2 ext4 ro,relatime,seclabel
[root@fedora boot]# findmnt /efi
TARGET SOURCE FSTYPE OPTIONS
/efi systemd-1 autofs rw,relatime,fd=94,pgrp=1,timeout=120,minproto=5,maxproto=5,direct,pipe_ino=9447
/efi /dev/vda1 vfat rw,nosuid,nodev,noexec,relatime,nosymfollow,fmask=0177,dmask=0077,codepage=437,iocharset=ascii,shortname=mixed,errors=remount-ro[root@fedora boot]# fdisk -l
Disk /dev/vda: 10 GiB, 10737418240 bytes, 20971520 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: gpt
Disk identifier: 78C1B509-AC00-4417-ADB9-8B2C1E481990
Device Start End Sectors Size Type
/dev/vda1 2048 2099199 2097152 1G EFI System
/dev/vda2 2099200 4196351 2097152 1G Linux extended boot
/dev/vda3 4196352 20969471 16773120 8G Linux root (x86-64)[root@fedora boot]# bootc status
[ 240.313722] EXT4-fs (vda3): re-mounted df0fd0e2-f9f1-4e2d-b90f-515d284dde04.
● Booted image: localhost/bootc-bls
Digest: sha256:026acb94e25c5b9a4c2f45a790377f8699434df90be3d033b6f1652cfaa524e3 (amd64)
Verity: 641f49373ed6d35f6b274d8e8216ba39e63aa4378d150f404a09ba54f200f46241b1a72639b966513912da3715c3258f9259c345168bfe1909d5ce98f2cbc718
Version: 44.20260908.0 (2026-09-09T03:29:24Z) |
|
Also, currently, we just put everything in the ESP (for SystemdBoot and GrubCC) so I don't think this would be an issue for us. @cgwalters hope I'm not missing some use case here? |
There was a problem hiding this comment.
🟡 Changes recommended
Upgrade paths still target /sysroot/boot, while mutating callers receive a read-only /boot handle.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates composefs GRUB boot-directory discovery for systems with a separately mounted /boot.
Changes:
- Detects whether
/bootis mounted. - Selects
/bootwhen it contains GRUB data; otherwise uses/sysroot/boot. - Inspects the mounted filesystem type with
findmnt.
File summaries
| File | Description |
|---|---|
crates/lib/src/store/mod.rs |
Adds runtime GRUB boot-directory selection. |
Review details
Suppressed comments (2)
crates/lib/src/store/mod.rs:470
- This selection is not propagated to the paths used to stage an upgrade.
setup_composefs_bls_boot()still hard-codes/sysrootforBootSetupType::Upgrade(bootc_composefs/boot.rs:748-750) and writes binaries and BLS configuration below/sysroot/boot(boot.rs:784-785, 892-908); the GRUB UKI path similarly derivesroot_path.join("boot")at line 1123. Consequentlybootc upgradein the linked separate-boot scenario still updates the stale physical-root directory. Thread the selected boot directory/path through both upgrade setup paths.
BootloaderKind::GRUBClassic => get_boot_dir_for_grub(&physical_root)?,
crates/lib/src/store/mod.rs:435
- This rejects every mounted
/bootfilesystem outside this hard-coded list, even when it contains the GRUB layout;--boot-mount-specimposes no corresponding filesystem restriction (install.rs:479-484, 2666-2673). Filesystem type is also not a reliable discriminator between ESP and XBOOTLDR. Probe for the expected GRUB/BLS directories and fall back to the physical-root boot directory instead of makingBootedStorage::new()fail on an otherwise valid mount.
fstype => {
anyhow::bail!("Unknown fstype {fstype} for /boot")
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if boot.is_dir("grub2") { | ||
| return Ok(boot); |
There was a problem hiding this comment.
Hmm right libostree has some code to handle this I think we do need to handle readonly XBOOTLDR (or classic mount)
| } | ||
|
|
||
| #[context("Finding boot for Grub")] | ||
| fn get_boot_dir_for_grub(physical_root: &Dir) -> Result<Dir> { |
|
|
||
| // XBOOTLDR, so grub configs should hopefully be here | ||
| // but check just in case | ||
| "ext4" | "xfs" | "btrfs" => { |
There was a problem hiding this comment.
I see no reason to hardcode this
| @@ -104,6 +105,7 @@ use cap_std_ext::cap_std::fs::{ | |||
| use cap_std_ext::dirext::CapStdExtDirExt; | |||
| use fn_error_context::context; | |||
|
|
|||
| use ocidir::cap_std::ambient_authority; | |||
| use ostree_ext::container_utils::ostree_booted; | |||
| use ostree_ext::prelude::FileExt; | |||
| use ostree_ext::sysroot::SysrootLock; | |||
| @@ -361,6 +363,80 @@ fn sysroot_is_read_only(d: &Dir) -> Result<bool> { | |||
| Ok(false) | |||
| } | |||
|
|
|||
| #[context("Finding boot for Grub")] | |||
| fn get_boot_dir_for_grub(physical_root: &Dir) -> Result<Dir> { | |||
| // We have this so systemd's boot.automount shouldn't expire until | |||
| // this function finishes execution as we have a handle to /boot | |||
| let boot = | |||
| Dir::open_ambient_dir("/boot", ambient_authority()).context("Failed to open /boot")?; | |||
|
|
|||
| let is_boot_mntpnt = boot | |||
| .is_mountpoint(".") | |||
| .context("Checking if /boot is a mountpoint")?; | |||
|
|
|||
| // /boot is not a mount point for bootloader Grub, so we have to | |||
| // have stuff in /sysroot/boot | |||
| if !matches!(is_boot_mntpnt, Some(true)) { | |||
| return physical_root | |||
| .open_dir("boot") | |||
| .context("Opening boot in physical root"); | |||
| } | |||
|
|
|||
| // /boot is a mountpoint | |||
| // Figure out if it's the ESP or XBOOTLDR | |||
| let mnt_res = run_findmnt(&[], None, Some("/boot")).context("Finding /boot mount info")?; | |||
|
|
|||
| let mut boot_fs = None; | |||
|
|
|||
| for mount in mnt_res.filesystems { | |||
| if mount.source.starts_with("systemd") { | |||
| // systemd automount, useless for getting any info | |||
| continue; | |||
| } | |||
|
|
|||
| if let Some(already_found) = boot_fs { | |||
| // Really shouldn't happen, but for sanity | |||
| anyhow::bail!( | |||
| "Found multiple mounts on /boot. Found {}, already had {already_found}", | |||
| mount.fstype | |||
| ); | |||
| }; | |||
|
|
|||
| boot_fs = Some(mount.fstype); | |||
| } | |||
|
|
|||
| let boot_fs = boot_fs.ok_or_else(|| anyhow::anyhow!("Failed to get filesystem for /boot"))?; | |||
|
|
|||
| // NOTE: It would be ideal here to check for DPS UUID but we can't be sure that the | |||
| // device that /boot is mounted as will have DPS compatible UUID | |||
If /boot is mounted as XBOOTLDR partition then grub configs are stored there as we were unconditionally searching for grub configs in
/sysroot/boot, which would fail every time. Fix this by checking if/bootis a mountpoint and then checking if we have grub configs in there.This is only an issue with Grub as GrubCC and SystemdBoot both do not store anything inside of
/sysroot/bootand will (should) always have the ESP mounted at /bootCloses: #2399