composefs: add check for UKI #199
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 adds a crucial check for a Unified Kernel Image (UKI) when the --composefs-backend option is used. The overall approach is correct. I've provided a couple of suggestions to improve the implementation: one to simplify the shell command used for the check in images.rs, and another to adjust the location of the check in to_disk.rs to ensure it also runs during a --dry-run for more accurate validation.
crates/kit/src/images.rs
Outdated
| let output = Command::new("podman") | ||
| .args(["run", "--rm", name, "sh", "-c", "ls /boot/EFI/Linux/*.efi 2>/dev/null | head -1"]) | ||
| .output()?; | ||
| Ok(output.status.success() && !output.stdout.is_empty()) |
There was a problem hiding this comment.
The shell command to check for the UKI file can be simplified. Instead of piping ls to head and checking stdout, you can rely on the exit code of ls. ls will fail with a non-zero exit code if no files match the glob. By redirecting stdout and stderr to /dev/null, you can simply check the command's success status. This is a more idiomatic and robust approach in shell scripting.
| let output = Command::new("podman") | |
| .args(["run", "--rm", name, "sh", "-c", "ls /boot/EFI/Linux/*.efi 2>/dev/null | head -1"]) | |
| .output()?; | |
| Ok(output.status.success() && !output.stdout.is_empty()) | |
| let status = Command::new("podman") | |
| .args(["run", "--rm", name, "sh", "-c", "ls /boot/EFI/Linux/*.efi >/dev/null 2>&1"]) | |
| .status()?; | |
| Ok(status.success()) |
crates/kit/src/to_disk.rs
Outdated
| if opts.install.composefs_backend && !images::has_uki(&opts.source_image)? { | ||
| return Err(eyre!( | ||
| "Image '{}' has no UKI - not suitable for --composefs-backend", | ||
| opts.source_image | ||
| )); | ||
| } |
There was a problem hiding this comment.
This check for a UKI is currently performed after the --dry-run logic has already completed and exited. A dry run should ideally perform all validations to accurately predict if the real operation would succeed. If the image is missing a UKI, the operation will fail, and a dry run should report this.
Please consider moving this validation block to the beginning of the run function (e.g., around line 372) so that it is executed for both normal and dry runs.
|
In theory we support both GRUB + Type1 BLS and systemd-boot + UKI for the pure composefs backend so it's not limited to UKIs. |
when --composefs-backend is used, make sure there is a UKI in the specified image. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
add-check-for-uki
branch
from
1856c28 to
04bf4c7
Compare
| } | ||
|
|
||
| /// Check if image has a UKI (required for --composefs-backend) | ||
| pub fn has_uki(name: &str) -> Result<bool> { |
There was a problem hiding this comment.
There's already code in this repo to parse the kernel state
| /// Main entry point for the bootc installation process. See module-level documentation | ||
| /// for details on the installation workflow and architecture. | ||
| pub fn run(opts: ToDiskOpts) -> Result<()> { | ||
| if opts.install.composefs_backend && !images::has_uki(&opts.source_image)? { |
There was a problem hiding this comment.
As @travier said this is strictly speaking not true, or to say it another way we definitely want to support it, but it is also true that we are not CI testing it or documenting it right now.
3 participants
when --composefs-backend is used, make sure there is a UKI in the specified image.
Assisted-by: Claude Code (Sonnet 4.5)