Integration for GrubCC and SystemdBoot#1113
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiple bootloaders, specifically adding a new GrubCC option alongside the default Grub. It updates the saved state, component installation, update, and status mechanisms to dynamically detect and filter metadata based on the active bootloader, including handling state files stored in the ESP for GrubCC. The review feedback identifies a potential mount leak where the was_mounted flag is not reset upon unmounting, several instances of unnecessary double referencing, and opportunities to make the code more idiomatic by simplifying OnceLock usage and removing redundant iterator adapters.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
2ffcc16 to
e8da148
Compare
77321d0 to
fb028b8
Compare
aa72462 to
e7bf29d
Compare
Now that we are going to support multiple bootloaders, we need a way to keep track of which one we're operating on Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Generate metadata for only a particular bootloader, either Grub or GrubCC Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Add `is_bootloader_supported` function to figure out if bootloader is supported by the component or not. Skip installs and metadata generation for the component if it does not support the bootloader Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
For grub we store the statefile in `/sysroot/boot/bootupd.json`. For grub-cc, and in future systemd-boot, we won't have a `/boot`, so we now store the state pre bootloader. Grub2: Statefile is still in `/sysroot/boot/bootupd.json` GrubCC: Statefile is stored in all the ESPs Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Store the sysroot_path in StateLockGuard which we require for mounting the ESP. Also, prevent mounting ESP at a tempdir, instead just mount it at `/boot` which is what the BLS spec suggest Fix issue with composefs systems where lsblk would fail to find the backing device for "/" due to it being mounted as a virtual filesystem. Instead we now use `/sysroot` or `/boot` to find the baking device Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
For `bootupd generate-update-metadata` we now generate metadata for all
found bootloaders. The final json will look like
```json
{
"timestamp": "2026-06-10T09:52:58.107743026Z",
"version": "grub-cc-1:2.12-59.fc45,grub2-1:2.12-58.fc44,shim-16.1-5",
"versions": [
{ "name": "grub-cc", "rpm_evr": "1:2.12-59.fc45" },
{ "name": "grub2", "rpm_evr": "1:2.12-58.fc44" },
{ "name": "shim", "rpm_evr": "16.1-5" }
]
}
```
This allows us to have multiple bootloaders in a single image and
install/update based on preference.
During install/update, we check to bootloader and filter out any and all
entries from the update metadata that do not match or are not required
for the bootloader
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Only allow Grub as the bootloader for powerpc and s390x Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
1c646b3 to
1106373
Compare
3a39aed to
4dd892b
Compare
|
I've tested |
This works exactly like GrubCC We expect systemd-boot efi to be in `/usr/lib/efi/systemd-boot/<evr>/fedora/grubx64.cc` Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We don't have e2e upgrade tests for now, but we can atleast test if the installations are working well Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We generate the final filetree from `/usr/lib/efi` which worked well with only one bootloader, but now that we have three which are stored in the following places Grub2 - /usr/lib/efi/grub2/<evr>/EFI/fedora/grubx64.efi GrubCC - /usr/lib/efi/grub-cc/<evr>/EFI/fedora/grubx64.efi SystemdBoot - /usr/lib/efi/systemd-boot/<evr>/EFI/fedora/grubx64.efi The problem arises as all the distinct efi binaries have the same name (grubx64.efi - due to the particular name being baked into shim), we cannot blindly create a filesystem tree out of this as later values will override the previous ones. To mitigate this, introduce a param `skip_dirs` which will filter directories not required for the bootloader being installed Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
A utility cfg macro defined in build.rs so that we can deny EFI only bootloaders - GrubCC and SystemdBoot, on non-EFI systems at compile time Also, only allow setting default bootloaders for EFI systems Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Use this enum in match statements so that if we add a new component in future, we'll get compile time errors in places where we need to handle the new component. This also gets rid of some hardcoded strings Also remove `#[default]` from Bootloader enum. Having a "default" in the enum doesn't make sense as we never construct this enum Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
|
A lot of changes here. I'll test it out in fcos as an override |
| grubName="grubaa64.efi" | ||
| fi | ||
|
|
||
| cp /usr/lib/systemd/boot/efi/systemd-boot*.efi "/usr/lib/efi/systemd-boot/${evr}/EFI/$ID/$grubName" |
There was a problem hiding this comment.
Hmmm, is this really required? I guess it must be - it's us taking over bootctl install and bootctl update.
That feels...unnecessary?
There was a problem hiding this comment.
Now that I spend a bit of time on this, is this case actually "install systemd-boot pretending to be grub" so it gets chainloaded by shim? That looks like what this is.
That feels like a clearly distinct case of things here and really worth clearly documenting.
OK right, and we're doing the same with grub-cc?
Hmmm, I guess it's kind of an assumption woven into things here that we're using shim, which is probably the right one by default, but I think we do ultimately need to handle non-shim cases too.
I guess this isn't blocking per se, but we should have a note about this.
There was a problem hiding this comment.
Now that I spend a bit of time on this, is this case actually "install systemd-boot pretending to be grub" so it gets chainloaded by shim? That looks like what this is.
yes, that's what it is. We will probably hardlink the actual systemd-boot binary at /usr/lib/efi/systemd-boot/** so that all bootloader live in the same place
OK right, and we're doing the same with grub-cc?
yes
but I think we do ultimately need to handle non-shim cases too
We do. On a call with @travier we decided to handle that case later
There was a problem hiding this comment.
The name of the EFI binary loaded by shim is hardcoded in shim: https://src.fedoraproject.org/rpms/shim-unsigned-x64/blob/rawhide/f/shim-unsigned-x64.spec#_154
There are options to change it but they have a lot of downsides: #1080 (comment)
Pretending to be grub$arch.efi is apparently the simplest option.
There was a problem hiding this comment.
We settled (at least for now) on leaving standalone systemd-boot (or potentially standalone grub-cc) support to systemd's bootctl as we don't need all the complexity from bootupd for those cases: #1080 (comment)
Rolv-Apneseth
left a comment
There was a problem hiding this comment.
Couple of questions and suggestions but wow, this is cool
| ;; | ||
| grub) | ||
| EFI_DIR_NAME=grub2 | ||
| ;; |
There was a problem hiding this comment.
Should we have a failure for the default case?
| use std::io::prelude::*; | ||
| use std::path::Path; | ||
|
|
||
| fn parse_statefile(statusf: cap_std::fs::File) -> Result<Option<SavedState>> { |
There was a problem hiding this comment.
WDYT about doing something like:
fn parse_statefile(statusf: cap_std::fs::File) -> Result<SavedState> {
let mut bufr = BufReader::new(statusf);
let mut s = String::new();
bufr.read_to_string(&mut s)?;
let state: serde_json::Result<SavedState> = serde_json::from_str(s.as_str());
match state {
Ok(s) => Ok(s),
Err(orig_err) => {
let legacy_state: serde_json::Result<crate::model_legacy::SavedState01> =
serde_json::from_str(s.as_str());
legacy_state
.map(crate::model_legacy::SavedState01::upconvert)
.map_err(|_| orig_err.into())
}
}
}And at the call sites:
root.open_optional(&path)
.map_err(Into::into)
.and_then(|o| o.map(parse_statefile).transpose())| // According to BLS, the ESP should be mounted at /boot or /boot/efi | ||
| // which the following method already checks | ||
| let mounted = efi | ||
| .ensure_mounted_esp(Path::new("/"), Path::new(&esp.path())) |
There was a problem hiding this comment.
Should this be using root_path in the case of an install to e.g. /var/mnt?
| root_path: impl AsRef<Path>, | ||
| bootloader: Bootloader, | ||
| ) -> Result<()> { | ||
| let saved_state = SavedState::load_from_disk(&root_path, Some(bootloader))?; |
There was a problem hiding this comment.
load_from_disk calls device.find_first_colocated_esp() for grub-cc and systemd-boot - do we need to handle the case where no ESP is found?
| // Do the sync before unmount | ||
| fsfreeze_thaw_cycle(dir.reopen_as_ownedfd()?)?; | ||
| drop(dir); | ||
| efi.unmount().context("unmount after update")?; |
There was a problem hiding this comment.
Is it intended that this unmounts even when was_mounted = true due to how Efi::unmount sets was_mounted = false?
| } else { | ||
| Bootloader::Grub | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| Some(info) => match info.to_lowercase() { | |
| i if i.contains("grub cc") => Bootloader::GrubCC, | |
| i if i.contains("systemd-boot") => Bootloader::Systemd, | |
| _ => Bootloader::Grub, | |
| }, |
| // | ||
| // We can't install GrubCC for BIOS as it's not supported | ||
| // So we just default to installing GrubCC | ||
| (Some(_), Some(eb)) => eb, |
There was a problem hiding this comment.
| (Some(_), Some(eb)) => eb, | |
| (_, Some(eb)) => eb, |
|
|
||
| /// Returns bootloaders are available for install | ||
| #[cfg(efi_arch)] | ||
| pub(crate) fn num_bootloader_available(&self) -> Vec<Bootloader> { |
There was a problem hiding this comment.
I think this should be renamed since it doesn't actually return a number
| #[cfg(efi_arch)] | ||
| pub(crate) fn bootloader_available(&mut self, bootloader: Bootloader) -> bool { | ||
| self.version | ||
| .split(",") | ||
| .any(|v| v.starts_with(bootloader.efi_component_name())) | ||
| } |
There was a problem hiding this comment.
| #[cfg(efi_arch)] | |
| pub(crate) fn bootloader_available(&mut self, bootloader: Bootloader) -> bool { | |
| self.version | |
| .split(",") | |
| .any(|v| v.starts_with(bootloader.efi_component_name())) | |
| } | |
| #[cfg(efi_arch)] | |
| pub(crate) fn is_bootloader_available(&self, bootloader: Bootloader) -> bool { | |
| self.version | |
| .split(',') | |
| .any(|v| v.starts_with(bootloader.efi_component_name())) | |
| } |
|
|
||
| . /etc/os-release | ||
|
|
||
| # Create the expected directory structure at /usr/lib/efi/grub-cc/<evr>/EFI/fedora/ |
There was a problem hiding this comment.
It looks like this was meant to be the systemd-boot path
Add Bootloader enum
Now that we are going to support multiple bootloaders, we need a way to
keep track of which one we're operating on
For
bootupd generate-update-metadatawe now generate metadata for allfound bootloaders. The final json will look like
{ "timestamp": "2026-06-10T09:52:58.107743026Z", "version": "grub-cc-1:2.12-59.fc45,grub2-1:2.12-58.fc44,shim-16.1-5", "versions": [ { "name": "grub-cc", "rpm_evr": "1:2.12-59.fc45" }, { "name": "grub2", "rpm_evr": "1:2.12-58.fc44" }, { "name": "shim", "rpm_evr": "16.1-5" } ] }This allows us to have multiple bootloaders in a single image and
install/update based on preference.
During install/update, we check to bootloader and filter out any and all
entries from the update metadata that do not match or are not required
for the bootloader
For grub we store the statefile in
/sysroot/boot/bootupd.json. Forgrub-cc, and in future systemd-boot, we won't have a
/boot, so we nowstore the state pre bootloader.
Grub2: Statefile is still in
/sysroot/boot/bootupd.jsonGrubCC: Statefile is stored in all the ESPs
Store the sysroot_path in StateLockGuard which we require for mounting
the ESP. Also, mount the ESP at
/bootwhich is what the BLS spec suggestSee: #1080