research: bootc-dev/bootc#2573 (operator request, cgwalters-forge/tracker#702) #23

Activity

1. cgwalters-bot commented on Oct 20, 2026

   

   cgwalters-bot

   on Oct 20, 2026

   Last edited by cgwalters-bot

   CollaboratorAuthor

   More actions

   ✅ agent ended well. Result: #23 (comment)
2. cgwalters-bot commented on Oct 20, 2026

   

   cgwalters-bot

   on Oct 20, 2026

   CollaboratorAuthor

   More actions

   Answers to the operator's review comments on bootc-dev/bootc#2573, checked against the PR head 1c1a9af. The author has already replied to every comment and pushed changes, so for each one this says whether those changes cover it and what is left.

   What I verified and what I didn't. I read the code at the head commit. I checked CI on that commit: every check passes, and the log of test-integration (fedora-44, composefs, xfs, systemd, uki, unsealed) shows plan-61-composefs-xbootldr: PASSED. I could not build or run the unit tests myself: there was no C toolchain on the machine I used. Anything below about how systemd or grub-cc behave comes from memory of those projects, not from testing, and is marked inferred.

   ---

   1. "Would be good to link to this in our ADOPTERS.md or so?"

   Done. The author opened #2576, which adds Luft (Vendor | Luft | 2026 | https://luft.computer | ...), and it is merged. Nothing more is needed in this PR.

   2. "Don't we have this in our DPS module? But that's also true of the ESP one above..." (r4298474187)

   Covered, with one leftover. Verified:

   • Commit 48ad556 moves crates/lib/src/discoverable_partition_specification.rs to crates/blockdev/src/dps.rs (99% similar) and exposes it as pub mod dps. The duplicate ESP and BIOS_BOOT constants in blockdev.rs are gone, and XBOOTLDR comes from dps.rs#L478.
   • All callers in bootc-lib (install.rs, install/baseline.rs, store/mod.rs) now use bootc_blockdev::dps. Nothing in the workspace used the removed bootc_blockdev::{ESP, BIOS_BOOT}. cfg-if and regex moved from bootc-lib's dependencies to blockdev's along with the module.

   Leftover: one copy of the BIOS boot GUID is still hard-coded in the sfdisk script at install/baseline.rs#L550, even though BIOS_BOOT is already imported in that file (L37). Suggested change, which could go into 48ad556:

   --- a/crates/lib/src/install/baseline.rs
   +++ b/crates/lib/src/install/baseline.rs
   @@ fn sfdisk(
            writeln!(
                &mut partitioning_buf,
   -            r#"size=1MiB, bootable, type=21686148-6449-6E6F-744E-656564454649, name="BIOS-BOOT""#
   +            r#"size=1MiB, bootable, type={BIOS_BOOT}, name="BIOS-BOOT""#
            )?;

   The constant is lowercase and the old literal is uppercase. I expect sfdisk to treat type GUIDs case-insensitively (inferred). The literal in the test_parse_lsblk assertion (blockdev.rs#L872) checks fixture data, so it is fine to leave as is.

   3. "I think what we really want here is a 'BLS conformant' path, which ideally grub-cc becomes too" (r4298499098)

   Mostly covered. Verified: the systemd-only check is gone. Choosing the partition now lives in bootc_composefs/boot_partition.rs, and every caller goes through it:

   • install: for_install
   • booted storage: mount_booted, called from store/mod.rs#L603
   • upgrade: Storage::require_boot_partition(), which reuses the choice made when storage was opened

   All of these depend on a single new predicate, Bootloader::is_bls_conformant().

   What is still subtle is that there are now two overlapping ideas side by side. BootloaderKind::BLSCompatible (systemd-boot and grub-cc) means "reads loader/entries". is_bls_conformant() (systemd-boot only) means "finds $BOOT the way the spec defines it: XBOOTLDR if present, otherwise the ESP". matches! won't flag a new Bootloader variant. I suggest an exhaustive match that spells out the relationship and the grub-cc gap, so that grub-cc "becoming conformant" really is a one-line change at a known place:

   --- a/crates/lib/src/spec.rs
   +++ b/crates/lib/src/spec.rs
   @@ impl Bootloader {
   -    /// Whether the bootloader reads boot entries from where the Boot Loader
   -    /// Specification puts them: the XBOOTLDR partition next to the ESP if
   -    /// there is one, and the ESP otherwise. grub-cc isn't known to read
   -    /// XBOOTLDR partitions yet, so it only gets entries on the ESP.
   +    /// Whether the bootloader finds `$BOOT` as the Boot Loader Specification
   +    /// defines it: the XBOOTLDR partition on the ESP's disk if there is one,
   +    /// and the ESP otherwise. Every BLS-conformant bootloader is
   +    /// [`BootloaderKind::BLSCompatible`], but not every BLSCompatible one is
   +    /// conformant yet. Only conformant bootloaders get their entries on an
   +    /// XBOOTLDR partition (see `BootPartition`).
        pub(crate) fn is_bls_conformant(&self) -> bool {
   -        matches!(self, Bootloader::Systemd)
   +        match self {
   +            Bootloader::Systemd => true,
   +            // TODO: flip once grub-cc's blscfg also reads entries from an
   +            // XBOOTLDR partition and resolves their paths on it.
   +            Bootloader::GrubCC => false,
   +            Bootloader::Grub | Bootloader::None => false,
   +        }
        }

   What a grub-cc that "becomes conformant" needs (inferred: I didn't test grub-cc against an XBOOTLDR partition):

   • On the grub-cc side. It is GRUB 2.12 with blscfg (per #2268). As far as I know it reads loader/entries only from the device it was loaded from. To be conformant it would have to:

     • find the XBOOTLDR partition on the same disk by its type GUID,
     • read entries from it as well as from the ESP,
     • resolve each entry's linux, initrd and efi paths on the partition that holds the entry.

   • On the bootc side, once grub-cc does that:

     • flip the arm above;
     • in the tmt test, replace the if $bootloader != "systemd" skip at test-composefs-xbootldr.nu#L236 with a check for systemd or grub-cc, so the existing grub-cc CI jobs cover it. Today the test skips itself in those jobs.

     The efi-versus-uki key handling in EFIKey::for_bootloader is the same on either partition, so it doesn't need to change.

   A smaller robustness point in the same module: has_type1_entry_for uses ? on parse_bls_config for every bootc_*.conf on the XBOOTLDR partition. One malformed bootc entry there makes opening booted storage fail, which breaks bootc status, upgrade and rollback. Skipping it seems safer, since the booted deployment's own entry is the one that matters:

   -            let entry = parse_bls_config(&entries.read_to_string(&name)?)?;
   +            let entry = match parse_bls_config(&entries.read_to_string(&name)?) {
   +                Ok(entry) => entry,
   +                Err(e) => {
   +                    tracing::warn!("Skipping {path}/{name}: {e:#}");
   +                    continue;
   +                }
   +            };

   4. "Are you creating /efi as a dir in your base image?" (r4298514310)

   The author's reply is right about the install-time mount. Verified: main already hard-codes esp_subdir = "boot" in MountedImageRoot, and the only stale part was the docstring promising <tmpdir>/efi. The PR fixes the docstring and replaces the TODO with a comment (boot.rs#L1974-L1979). That mount is only used for bootctl install --esp-path, and bootc writes the entries itself.

   Where /efi does matter is the booted system, which neither the reply nor the docs mention (inferred from how systemd behaves, not tested). With an XBOOTLDR partition, systemd's tools expect XBOOTLDR at /boot and the ESP at /efi. That covers bootctl status/list, boot counting via systemd-bless-boot, and systemd-gpt-auto-generator's automounts. On a read-only composefs root without an /efi directory, the ESP has nowhere to be mounted, so those tools see only part of the setup. bootc itself is unaffected, because it mounts both partitions on its own (mount_booted). Suggested addition to the new docs section (docs/src/bootc-bootloaders.7.md):

    ... A system whose boot entries are on the ESP keeps them there, even if an XBOOTLDR partition is added later.
   +
   +bootc mounts both partitions itself when it needs them. For systemd's own
   +tools (e.g. `bootctl`) to see the layout the Boot Loader Specification
   +describes, with XBOOTLDR on `/boot` and the ESP on `/efi`, the image needs an
   +empty `/efi` directory (`RUN mkdir /efi`), since the root filesystem is
   +read-only at runtime.

   Whether fedora-bootc/centos-bootc should ship /efi by default is a question for the base images. I did not check their contents.

   5. (Later) "our logic here must indeed handle the case where detect bootloader entries on the ESP but not XBOOTLDR and prefer that" (r4298544901)

   Covered. Verified in the code:

   • mount_booted uses XBOOTLDR only if it holds a bootc_*.conf entry, in loader/entries or loader/entries.staged, whose verity matches the booted composefs= digest. Otherwise it falls back to the ESP.
   • UKI deployments are covered as well, because write_systemd_uki_config also writes a .conf entry whose efi/uki key get_verity() parses.
   • The chosen partition is stored in Storage, so upgrade writes and GC use the same one.
   • The tmt test's second boot starts with an empty XBOOTLDR partition, runs bootc status, and checks that bootc switch stages onto the ESP. Only after that does it move the files over. That test passes in CI (see the top).

   Two things I'd raise with the author, neither blocking:

   • On an XBOOTLDR install, loader/loader.conf (timeout 5) is never written (boot.rs#L1766), because only the boot partition is mounted there. Is that intended? If so, a sentence in the docs would help.
   • Storage::esp and require_esp() now return the boot partition, which may be XBOOTLDR. The doc comments were updated, but a rename (e.g. boot_mount) in a follow-up would make the GC and UKI callers (gc.rs, utils.rs, bcompat_boot.rs) easier to read correctly.

   Generated by agent · ◷