composefs-native/uki: Handle UKI addons #1573
Conversation
|
I thought |
![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 refactors the UKI boot setup to handle UKI addons by processing a list of boot entries instead of a single one. A new helper function, write_pe_to_esp, is introduced to encapsulate writing PE files to the ESP, which is a good separation of concerns. The changes are generally well-structured and address the goal of handling UKI addons. I've identified a couple of areas for improvement regarding code consistency and robustness. My detailed feedback is in the comments below.
jeckersb
force-pushed
the
composefs-backend
branch
from
fd703ec to
023be10
Compare
Johan-Liebert1
force-pushed
the
composefs-backend
branch
from
bda61be to
389a922
Compare
Johan-Liebert1
force-pushed
the
uki-addons
branch
from
5a03150 to
c59c776
Compare
Johan-Liebert1
added
area/composefs
Johan-Liebert1
force-pushed
the
uki-addons
branch
from
c59c776 to
cf6b5c8
Compare
If we find UKI addons in the boot entries list, write them to ESP along with the UKI Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We don't need to write Grub menuentries for systemd-boot. For now the operation is a no-op, but later we would want to have .conf files in `ESP/loader/entries` so we can control the order of entries. Regarding that, we would also need to place the UKIs in a separate directory and not inside `ESP/EFI/Linux`, if we don't want duplicate entries, as systemd-boot will simply list all .efi files placed in EFI/Linux unconditionally Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
uki-addons
branch
from
cf6b5c8 to
caf4ad8
Compare
|
Rebased and fix conflicts. @cgwalters could I have a review? |
|
|
||
| for entry in entries { | ||
| match entry { | ||
| ComposefsBootEntry::Type1(..) => tracing::debug!("Skipping Type1 Entry"), |
There was a problem hiding this comment.
FWIW I basically don't think bootc should support this. The main use case is kernel arguments which we already cover with bootc kargs.
So my preference going forward is a hard error, not just a debug.
| match entry { | ||
| ComposefsBootEntry::Type1(..) => tracing::debug!("Skipping Type1 Entry"), | ||
| ComposefsBootEntry::UsrLibModulesVmLinuz(..) => { | ||
| tracing::debug!("Skipping vmlinuz in /usr/lib/modules") |
There was a problem hiding this comment.
Shouldn't this one also be an error?
There was a problem hiding this comment.
Currently, using existing images to create UKIs also has kernel+initrd in /usr/lib/modules. But, yes, makes sense for these to be errors as we only ever want one type of boot entry. But, let's say, if an image does have both type of entries, as users could be using the same image, should we still throw an error?
2 participants
If we find UKI addons in the boot entries list, write them to ESP along with the UKI
Right now if a UKI Addon also has the
composefs=cmdline param, it's ignored.Supporting: containers/composefs-rs#126
Needs containers/composefs-rs#178