Improve set/get cmdline to support uefi #4062
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR improves the GRUB bootloader support in sonic-installer to handle UEFI systems by adding support for the linuxefi command in addition to the existing linux command. This fixes an issue where sonic-installer get-fips/set-fips commands were not working on UEFI systems.
- Added support for
linuxeficommand in cmdline parsing and modification - Maintained backward compatibility with legacy
linuxcommand - Fixed FIPS configuration on UEFI systems
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
sonic_installer/bootloader/grub.py
Outdated
| if line.startswith('linuxefi '): | ||
| cmdline = line[9:].strip() | ||
| break | ||
| elif line.startswith('linux '): | ||
| cmdline = line[6:].strip() | ||
| break |
There was a problem hiding this comment.
The magic numbers 9 and 6 represent the length of 'linuxefi ' and 'linux ' respectively. Consider extracting these as named constants (e.g., LINUXEFI_PREFIX_LEN = len('linuxefi ')) to improve code readability and maintainability.
Copilot uses AI. Check for mistakes.
sonic_installer/bootloader/grub.py
Outdated
| if line.startswith('linuxefi '): | ||
| new_menuentry = old_menuentry.replace(line, "linuxefi " + cmdline) | ||
| break | ||
| elif line.startswith('linux '): | ||
| new_menuentry = old_menuentry.replace(line, "linux " + cmdline) | ||
| break |
There was a problem hiding this comment.
Similar to the get_linux_cmdline method, the hardcoded prefix strings 'linuxefi ' and 'linux ' should be extracted as constants to maintain consistency and reduce duplication across both methods.
Copilot uses AI. Check for mistakes.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Updated test to use new temporary host path for EFI.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@stepanblyschak , can you review this PR? |
Improve set/get cmdline to support uefi
What I did
Fix sonic-net/sonic-buildimage#23820
sonic-installer get-fips/set-fips not working on systems in UEFI mode
How I did it
Improve get_linux_cmdline and set_linux_cmdline to support linuxefi
How to verify it
Manually verify.
Pass all test case
Work item tracking
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)