composefs/boot: Fix sd-boot order on update #1779
Conversation
|
I'm also working on adding tests for these cases |
![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 fixes the boot order for sd-boot during an update by setting the correct sort-key for BLS entries. The change correctly handles the different sorting behaviors of Grub (descending) and systemd-boot (ascending). The logic is sound. I've added one suggestion to refactor duplicated logic to improve code maintainability.
Johan-Liebert1
force-pushed
the
sdboot-order
branch
from
f427fb7 to
3222da9
Compare
Grub sorts its BLS config in descending order, while sd-boot sorts the configs in ascending order. While upgrading we were always setting the new sort key to be `1` which would work for Grub but not for sd-boot. Fixes: bootc-dev#1777 Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
sdboot-order
branch
from
3222da9 to
ad86fc8
Compare
| .context("Computing boot digest")?; | ||
|
|
||
| let default_sort_key = "1"; | ||
| let default_sort_key = bootloader.default_sort_key(); |
There was a problem hiding this comment.
Hmm I think it'd be cleaner if computed all the BLS entries without writing them, and then walk over all of them and reverse the order if we detect grub or so?
There was a problem hiding this comment.
That is kind of what we're doing, but in a bit of a weird way. This might be good for a cleanup. I'll pick it up
| pub(crate) const GRUB_SECONDARY_SORT_KEY: &str = "0"; | ||
|
|
||
| /// Sort key for the primary BLS entry | ||
| pub(crate) fn default_sort_key(&self) -> &str { |
There was a problem hiding this comment.
How about
sorting_keys(&self) -> impl Iterator<Item=&'static str> {
let r = ["0", "1"]
match self {
Systemd => r
Grub => r.rev()
}
There was a problem hiding this comment.
We use the secondary keys when upgrading/switching, so this doesn't quite work for that case
|
OK first when talking about grub, there's a giant mess there because it has a wildly varying patch set per distro/OS. In particular https://src.fedoraproject.org/rpms/grub2 now has 390 (!) patches. Anyways I think the real problem here is: i.e. at least Fedora's grub2 isn't implementing We should make this at least build or runtime configurable probably in the future, but for now until we gather more data on grub on other OSes it's likely simplest to just not emit |
|
From what I can see, this patch doesn't change the current behaviour for Grub, but fixes the currently broken behaviour for systemd-boot. Is it worth getting this in first and then dealing with Grub as a separate PR if more info is needed before figuring out the best solution for Grub? |
That's what's happening, at least in fedora based images. The |
Yes I think that's true, however I think it's well worth some further cleanups here. We'll get this fixed soon. |
3 participants
Grub sorts its BLS config in descending order, while sd-boot sorts the configs in ascending order. While upgrading we were always setting the new sort key to be
1which would work for Grub but not for sd-boot.Fixes: #1777