install: refactor console handling and disable GRUB on s390x#1736
Conversation
Extract kernel argument configuration logic from write_disk() into a new configure_kernel_arguments() function. This improves code organization, no functional changes.
Extract GRUB console configuration into configure_grub() function, called separately after kernel arguments are configured in write_disk(). This separates GRUB settings from kernel argument handling.
Add cfg guards to disable GRUB-related functions on s390x since GRUB is not used on that architecture.
![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 refactors the installation process to correctly handle the '--console' flag and disable GRUB configuration on s390x architectures. The changes introduce several helper functions for kernel argument and GRUB configuration management, utilizing conditional compilation to ensure platform-specific behavior. Review feedback identifies opportunities to improve performance by reducing redundant disk I/O, specifically by consolidating multiple reads of 'platforms.json' and minimizing sequential writes to the BLS configuration file through a single 'visit_bls_entry_options' call.
nikita-dubrovskii
changed the title
|
Conflicts with: #1732. This PR adds further refactoring to clean up the code and better separate the logic. Also, while |
There was a problem hiding this comment.
I'm not the biggest fan of including ~10 #[cfg(not(target_arch = "s390x"))] statements in here mainly for code readability. Can we minimize it to just 2?
- line 430
- line 932
I realize this probably makes the binary size smaller, but how much does it really save? It would have to be a lot for it to be worth it I think.
There was a problem hiding this comment.
In such case there will be a lot of warnings like:
warning: function `build_grub_console_commands` is never used
There was a problem hiding this comment.
I think we should include them as it makes it clear that those functions are not used on a specific arch. It matches what I've seen in other Rust projects AFAIK.
There was a problem hiding this comment.
If we want to minimize it further then we could split the code into another file and set the conditionnal there.
There was a problem hiding this comment.
ok. man this really makes the code ugly though.
travier
left a comment
travier
left a comment
There was a problem hiding this comment.
I did not test this code but the style and implementation LGTM.
3 participants
On s390x, installations with
--consolewould fail because the installer attempted to modify GRUB config files that don’t exist (s390xuseszipl).This PR refactors the code to better separate
--platformand--consolelogic, and disables all GRUB-related code paths fors390x.Issue: #1171