Skip to content

Conversation

Johan-Liebert1
Copy link
Collaborator

@Johan-Liebert1 Johan-Liebert1 commented Sep 2, 2025

While collecting UKI from /boot/EFI/Linux or /usr/lib/modules we now also collect UKI addons from any directory that ends with .efi.extra.d

xref #126

Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

Looking good. Not tested.

@Johan-Liebert1
Copy link
Collaborator Author

I've integrated this with bootc locally and things work. We probably need tests in here as well

While collecting UKI from /boot/EFI/Linux or /usr/lib/modules we now
also collect UKI addons from any directory that ends with
`.efi.extra.d`

Signed-off-by: Johan-Liebert1 <[email protected]>
Copy link
Collaborator

@allisonkarlitskaya allisonkarlitskaya left a comment

Choose a reason for hiding this comment

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

Closes #126

pub file: RegularFile<ObjectID>,
pub pe_type: PEType,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing it this way seems a bit weird. The addons are always associated with a specific kernel image, right? I was imagining that we'd add something like a boxed slice of RegularFile to each Type2Entry so we can see which addons go with with kernel.

Or did I miss something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah we could definitely do it by adding a Vec of RegularFile to a single Type2Entry, but apparently there are global addons as well which can be placed in $BOOT/loader/addons/

@@ -207,41 +209,88 @@ impl<ObjectID: FsVerityHashValue> Type1Entry<ObjectID> {
}
}

pub const EFI_EXT: &str = ".efi";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually prefer to just put these sort of things closer to where they're used or just write them directly...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use them in bootc as well, so thought making these constants made sense

@Johan-Liebert1
Copy link
Collaborator Author

@allisonkarlitskaya I'm thinking about merging Type2Entry and UsrLibModulesUki as they both serve the exact same function with the only difference being the path that they both traverse. Is there any reason we'd like to keep them separate?

Merge `Type2Entry` and `UsrLibModulesUki` structs keeping only
`Type2Entry`

Signed-off-by: Johan-Liebert1 <[email protected]>
@Johan-Liebert1
Copy link
Collaborator Author

Merged Type2Entry and UsrLibModulesUki keeping only Type2Entry. Also, handle collecting global UKI addons inside of loader/addons

pub fn load_all(root: &Directory<ObjectID>) -> Result<Vec<Self>> {
let mut entries = vec![];

match root.get_directory("/boot/EFI/Linux".as_ref()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait isn't this the /usr/lib/modules case? Why are we looking in /boot?

This seems to be exacerbating a long-running debate about the role of /boot in container images.

https://uapi-group.org/specifications/specs/unified_kernel_image/#locations-and-naming-for-uki-auxiliary-resources seems very clear that this content should be in /usr/lib/ and not in /boot in images.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is more to do with how currently we only ignore /boot (and now /sysroot) in our EROFS image generation. I believe this has been a long running on our TODO list

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason we mask /boot for the primary UKI is to break the circular dependency with the composefs digest.

But that's not a problem for auxiliary UKI resources right? Although it's kind of interesting because I guess in theory actually one could have the expected composefs digest as an auxiliary UKI resource instead...that's an interesting thought because it helps break up the "build" of sealed image....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Auxiliary UKI resources can live anywhere, but usually they'll be alongside the UKI they extend, at least that's what I think. Of course we'll need to mask the directory the addon is in if it has composefs cmdline in it.

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

Successfully merging this pull request may close these issues.

4 participants