Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for UKI addons by detecting a per-UKI addon directory in the source image and staging addon artifacts onto the ESP alongside the staged UKI, with follow-up renaming during boot order update.
Changes:
- Detect
<UKI filename>.extra.d/in the source image and copy addon artifacts to the ESP during UKI staging. - Introduce naming constants for addon directories/files and extend staging logic to ignore non-matching entries.
- Attempt to rename a staged addon directory when renaming the staged UKI to its final name.
| } | ||
| } |
There was a problem hiding this comment.
Issue: When updating/replacing an existing UKI for the same uki_suffix, the function removes only the UKI file but does not remove the associated addon directory (<UKI filename>.extra.d). This can leave stale addon artifacts on the ESP across updates.
Evidence: In the for (index, suffix, path) in existing_ukis { if suffix == uki_suffix { ... fs::remove_file(&path) ... } } block earlier in this function, there is no corresponding cleanup of path + .extra.d.
Suggestion: When deleting an old UKI file for the same suffix, also delete its addon directory if present (e.g., remove_dir_all on <old_uki_path>.extra.d).
| if !path.ends_with(UKI_ADDON_FILE_SUFFIX) { | ||
| trace!( | ||
| "Ignoring file '{}' in addon directory that does not match expected naming scheme", | ||
| path.display() | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Issue: Path::ends_with(UKI_ADDON_FILE_SUFFIX) checks whether the path ends with a path component, not whether the file name ends with a string suffix. For an addon file like foo.addon.efi, this condition will be false and the file will be incorrectly skipped (only a file literally named .addon.efi would match).
Evidence: The addon filter uses if !path.ends_with(UKI_ADDON_FILE_SUFFIX) { ... continue; }.
Suggestion: Perform the suffix check on the file name string (e.g., path.file_name() -> string -> .ends_with(UKI_ADDON_FILE_SUFFIX)) rather than using Path::ends_with.
frhuelsz
changed the title
|
|
||
| /// Enumerates preexisting UKIs unmanaged by Trident (defined by naming convention: vmlinuz-6.6.117.1-1.azl3.efi) | ||
| /// in the given directory, returning their versions and paths. | ||
| fn enumerate_non_trident_managed_ukis( |
There was a problem hiding this comment.
i imagine we'd need to watch for the add-on folder here too?
|
maybe |
| let addon_dir = uki_addon_dir(&path); | ||
| if addon_dir.exists() && addon_dir.is_dir() { | ||
| fs::remove_dir_all(&addon_dir) | ||
| .with_context(|| { | ||
| format!("Failed to remove addon directory '{}'", addon_dir.display()) | ||
| }) | ||
| .structured(ServicingError::UpdateUki)?; | ||
| } |
There was a problem hiding this comment.
The PR description mentions "TODO: cleanup old addon dirs when the corresponding UKI is removed." However, this functionality is already implemented in the code at lines 254-261, where addon directories are removed when their corresponding UKI files are removed. Consider updating the PR description to reflect that this TODO has been completed, or clarify if there's additional cleanup work still needed.
| fs::create_dir_all(&dest_addon_dir).context("Failed to create destination addon directory")?; | ||
|
|
||
| // Copy all files from the source addon directory to the destination addon | ||
| // directory. We expect these to be files with names ending in `.addon.efi`. | ||
| for entry in fs::read_dir(&addon_dir).context("Failed to read addon directory")? { | ||
| let entry = entry.context("Failed to read entry in addon directory")?; | ||
| let path = entry.path(); | ||
| if !path.is_file() { | ||
| trace!( | ||
| "Ignoring non-file entry '{}' in addon directory", | ||
| path.display() | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| if !path | ||
| .as_os_str() | ||
| .as_encoded_bytes() | ||
| .ends_with(UKI_ADDON_FILE_SUFFIX.as_bytes()) | ||
| { | ||
| trace!( | ||
| "Ignoring file '{}' in addon directory that does not end with expected suffix '{UKI_ADDON_FILE_SUFFIX}'", | ||
| path.display(), | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| let dest_addon_file = | ||
| dest_addon_dir.join(path.file_name().context("Failed to get addon file name")?); | ||
|
|
||
| debug!( | ||
| "Staging UKI addon file from '{}' to '{}'", | ||
| path.display(), | ||
| dest_addon_file.display() | ||
| ); | ||
| fs::copy(&path, &dest_addon_file).context("Failed to copy addon file")?; | ||
| } |
There was a problem hiding this comment.
When an addon directory exists but contains no valid .addon.efi files, an empty destination addon directory is created and left on the ESP. Consider whether this is the desired behavior. If not, you could track whether any files were actually copied and remove the empty directory if none were found.
This is a minor edge case that may not matter in practice, but it could lead to empty directories accumulating on the ESP over time if images are built with empty addon directories.
2 participants
🔍 Description
Scan for matching addon directories and copy artifacts as needed.
TODO: cleanup old addon dirs when the corresponding UKI is removed.