Skip to content

Conversation

Johan-Liebert1
Copy link
Collaborator

Fix Grub boot error caused by #1541. Introduce a --bootloader cli option to --composefs-native. Depending upon the type of bootloader passed in we write BLS configs respectively

@bootc-bot bootc-bot bot requested a review from cgwalters September 1, 2025 11:55
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for both systemd-boot and grub by adding a --bootloader option, which is a great enhancement. The changes are well-structured, particularly the use of the BLSEntryPath struct to handle bootloader-specific paths. I've identified a critical issue with temporary directory handling that could lead to resource leaks, and a high-severity issue regarding backward compatibility for existing deployments. Addressing these will make the implementation more robust.

@Johan-Liebert1 Johan-Liebert1 force-pushed the grub-bls-fix branch 2 times, most recently from 0838456 to fa58def Compare September 8, 2025 07:08
Fix Grub boot error caused by
bootc-dev#1541. Introduce a `--bootloader`
cli option to `--composefs-native`. Depending upon the type of
bootloader passed in we write BLS configs respectively

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>

Add guard again tempdir drop

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as is however...I still think we should detect grub instead via looking for its config files

@cgwalters cgwalters merged commit ab40a99 into bootc-dev:composefs-backend Sep 8, 2025
12 of 21 checks passed
@cgwalters cgwalters added the area/composefs Issues related to composefs label Sep 8, 2025
@Johan-Liebert1
Copy link
Collaborator Author

I still think we should detect grub instead via looking for its config files

Is there some peculiar reason why we should be looking for grub config files? I think the EFI vars approach is a bit better as it also lets us distinguish BIOS/UEFI

@cgwalters
Copy link
Collaborator

Is there some peculiar reason why we should be looking for grub config files? I think the EFI vars approach is a bit better as it also lets us distinguish BIOS/UEFI

In theory we should work with any bootloader that implements the spec instead of specifically detecting systemd-boot - right?

@Johan-Liebert1
Copy link
Collaborator Author

Right. AFAIK bootloaders on UEFI will populate the LoaderInfo file with the bootloader name, which we check right now. Though, we do default to Grub if we can't find /sys/firmware/efi/efivars, in which case it does become necessary to look for bootlaoder specific config files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/composefs Issues related to composefs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants