Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Move UEFI detection to utils_misc #3849

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

veedlaw
Copy link

@veedlaw veedlaw commented Feb 22, 2024

Previously, the UEFI guest detection functions were part of the specific VMCheck classes (LinuxVMCheck and WindowsVMCheck) in the virttest/utils_v2v.py file. This commit refactors and relocates these functions to the more generic virttest/utils_misc.py file.

Motivation:

  • The is_uefi_guest function, previously defined in the VMCheck classes, had dependencies that were not universally available across different architectures (e.g., ovrit on s390x).
  • To enhance code reusability the UEFI guest detection functions are moved to the utils_misc module.

Updated Functions:

  • is_linux_uefi_guest(runner): Check if a Linux guest is a UEFI guest.
  • is_windows_uefi_guest(runner): Check if a Windows guest is a UEFI guest.

These changes improve the code reusability in the codebase and make the UEFI guest detection functionality more accessible for various scenarios.

Previously, the UEFI guest detection functions were part of the specific VMCheck classes (`LinuxVMCheck` and `WindowsVMCheck`) in the `virttest/utils_v2v.py` file. This commit refactors and relocates these functions to the more generic `virttest/utils_misc.py` file.

Motivation:
- The `is_uefi_guest` function, previously defined in the `VMCheck` classes, had dependencies that were not universally available across different architectures (e.g., ovrit on s390x).
- To enhance code reusability the UEFI guest detection functions are moved to the utils_misc module.

Updated Functions:
- `is_linux_uefi_guest(runner)`: Check if a Linux guest is a UEFI guest.
- `is_windows_uefi_guest(runner)`: Check if a Windows guest is a UEFI guest.

These changes improve the code reusability in the codebase and make the UEFI guest detection functionality more accessible for various scenarios.
Copy link
Contributor

@smitterl smitterl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small nitpick about an unused variable. Rest LGTM, thank you.

virttest/utils_misc.py Outdated Show resolved Hide resolved
virttest/utils_misc.py Outdated Show resolved Hide resolved
virttest/utils_misc.py Outdated Show resolved Hide resolved
Copy link
Contributor

@smitterl smitterl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this. Your commits are missing the "Signed-off-by" line, you can add this with git commit --amend -s after configuring user name and email e.g. via git config --edit --global.

There are two lines too much in the windows function, IMO, please check the comment.

cmd = 'findstr /c:"%s" %s' % (search_str, target_file)
status, output = runner(cmd)
if 'BIOS' in output:
return False
Copy link
Contributor

@smitterl smitterl Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are not necessary, you already return 'EFI' in output. Please, remove them. Also, you don't use status, so please change to _, output = runner(cmd).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes pushed.

…efi guest for clarity

Signed-off-by: Walter Herold Veedla <walterheroldv@gmail.com>
@veedlaw veedlaw force-pushed the refactor-is_uefi_guest-from-utils_v2v-to-utils_misc branch from 40566f9 to 37a91ac Compare March 17, 2024 11:07
@veedlaw veedlaw requested a review from smitterl March 27, 2024 07:54
Copy link
Contributor

@smitterl smitterl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants