install: Fix --console flag on s390x#1732
install: Fix --console flag on s390x#1732nikita-dubrovskii wants to merge 1 commit intocoreos:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where using the --console flag on s390x would cause the installation to fail. The fix correctly skips modifying the non-existent GRUB configuration on s390x and instead applies the console kernel arguments to the BLS entries. The changes look correct and effectively address the issue. I've added one suggestion to refactor the kernel argument modifications to be more efficient by combining multiple file I/O operations into one.
| visit_bls_entry_options(mount.mountpoint(), |orig_options: &str| { | ||
| KargsEditor::new() | ||
| .append_if_missing(console_kargs.as_slice()) | ||
| .maybe_apply_to(orig_options) | ||
| }) | ||
| .context("appending console kargs")?; |
There was a problem hiding this comment.
This introduces a second call to visit_bls_entry_options, which involves reading from and writing to the BLS entry file. The call for append-karg and delete-karg on lines 448-454 and this new call for console kargs could be combined into a single operation to improve efficiency and maintainability.
You could build a KargsEditor instance and apply all modifications in one go. Here's a suggested refactoring for lines 441-471:
let mut kargs_editor = KargsEditor::new();
if !config.append_karg.is_empty() || !config.delete_karg.is_empty() {
eprintln!("Modifying kernel arguments");
// On s390x there is no GRUB config, so skip this check to avoid a user warning.
#[cfg(not(target_arch = "s390x"))]
Console::maybe_warn_on_kargs(&config.append_karg, "--append-karg", "--console");
kargs_editor
.append(config.append_karg.as_slice())
.delete(config.delete_karg.as_slice());
}
#[cfg(target_arch = "s390x")]
{
// Since there's no GRUB config, set console kargs by updating the BLS config
if !config.console.is_empty() {
eprintln!("Adding console kernel arguments");
let console_kargs = config.console.iter().map(|c| c.karg()).collect::<Vec<_>>();
kargs_editor.append_if_missing(console_kargs.as_slice());
}
}
if kargs_editor != KargsEditor::default() {
visit_bls_entry_options(mount.mountpoint(), |orig_options: &str| {
kargs_editor.maybe_apply_to(orig_options)
})
.context("modifying kernel arguments")?;
}This would consolidate all kernel argument modifications before the s390x::zipl call.
|
Checked on s390x, |
On s390x, the installer was attempting to modify GRUB config that doesn't exist. This caused installations with --console to fail. Fix by skipping write_console() on s390x and instead applying console kernel arguments to BLS entries." Issue: coreos#1171
b5df649 to
e2ee439
Compare
On s390x, the installer was attempting to modify GRUB config that doesn't exist. This caused installations with
--consoleto fail. Fix by skippingwrite_console()on s390x and instead applying console kernel arguments to BLS entries."Issue: #1171