Repository navigation
efi: Extend get_efi_vendor() to support usr/lib/efi - #991
Conversation
![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 extends get_efi_vendor() to support usr/lib/efi as a new location for EFI files. The core change involves refactoring several functions, notably install() and get_efi_vendor() across components, to accept a path string (&str) instead of an openat::Dir. This is a sensible refactoring that enables more flexible path handling. The new logic in get_efi_vendor() correctly prioritizes the new path and is accompanied by thorough tests. Overall, the changes are well-implemented and consistent. I have one suggestion to improve code clarity.
jbtrystram
left a comment
jbtrystram
left a comment
There was a problem hiding this comment.
Superficial LGTM as I don't know this codebase well, but the change look sane. Suggested a small change to avoid having to convert all the openat::Dir to str all around maybe ?
0e41d17 to
17c571c
Compare
Check we do not have it in latest 9.0.20250828-0-coreos: ``` $ podman run -q --rm -it quay.io/openshift-release-dev/ocp-v4.0-art-dev:9.0.20250828-0-coreos ls /var lib run tmp ```
17c571c to
2b25197
Compare
|
I will merge this if there is no objection. |
| # Clean out the default to ensure we're using our updated content | ||
| RUN rpm -e bootupd | ||
| COPY --from=build /out/ / | ||
| # Remove /var/roothome as workaround |
There was a problem hiding this comment.
What error is this working around?
| { | ||
| p | ||
| } else { | ||
| bail!("Failed to find valid target path"); | ||
| }; |
There was a problem hiding this comment.
This is just .ok_or_else(|| anyhow!("Failed..."))
| .sub_dir(&component_updatedirname(self)) | ||
| .context("opening update dir")?; | ||
| let shim_files = find_file_recursive(updated.recover_path()?, SHIM)?; | ||
| fn get_efi_vendor(&self, sysroot: &str) -> Result<Option<String>> { |
There was a problem hiding this comment.
I don't follow why we needed to switch to taking an ambient path here.
There was a problem hiding this comment.
Will try to update to Path later, just thought it could be convenient to pass.
4 participants
Prepare for step2 (in #926) to install efi files from
usr/lib/efi