test teview - #1
Conversation
Signed-off-by: Yingshun Cui <yicui@redhat.com>
|
/gemini review |
![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
The pull request adds a new test file for VM lifecycle testing with a VFIO variant driver. The review identifies several issues in the new test file, following Python's general best practices. These include a critical security vulnerability due to the use of eval(), a bug in the test cleanup logic, and several opportunities to improve code maintainability and robustness by removing code duplication, using the test framework's features for temporary files, and centralizing parameter access.
| hotplug = params.get("hotplug", "no") == "yes" | ||
| err_msg = params.get("err_msg") | ||
| managed = params.get("managed") | ||
| iommu_dict = eval(params.get("iommu_dict", "{}")) |
There was a problem hiding this comment.
Using eval() on input from parameters is a significant security risk as it can execute arbitrary code. For safely parsing string-formatted Python literals, ast.literal_eval() should be used instead.
Note: This change requires adding import ast at the top of the file.
| iommu_dict = eval(params.get("iommu_dict", "{}")) | |
| iommu_dict = ast.literal_eval(params.get("iommu_dict", "{}")) |
|
|
||
| finally: | ||
| orig_vm_xml.sync() | ||
| if managed == "no": |
There was a problem hiding this comment.
The condition managed == "no" is too strict. If the managed parameter is not provided in the test configuration, params.get("managed") will return None, and this condition will be false. However, when the managed attribute is omitted from the device XML, libvirt defaults to not managing the device, which means it should be reattached by the test script.
To correctly handle the default case (and any value other than 'yes'), the condition should be managed != "yes".
| if managed == "no": | |
| if managed != "yes": |
| vm_name = params.get("main_vm", "avocado-vt-vm1") | ||
| vm = env.get_vm(vm_name) | ||
| rand_id = utils_misc.generate_random_string(3) | ||
| save_path = f'/var/tmp/{vm_name}_{rand_id}.save' |
There was a problem hiding this comment.
The save path is hardcoded to /var/tmp. It's better practice to use the test's working directory, which is managed by the test framework (Avocado). This ensures files are created in a temporary, isolated location and are properly cleaned up. You can use test.workdir for this.
Note: This change requires adding import os at the top of the file to use os.path.join.
| save_path = f'/var/tmp/{vm_name}_{rand_id}.save' | |
| save_path = os.path.join(test.workdir, f'{vm_name}_{rand_id}.save') |
| if hotplug: | ||
| vm.start() | ||
| vm.wait_for_serial_login().close() | ||
| dev_names = sriov_vfio.attach_dev(vm, params) | ||
| else: | ||
| dev_names = sriov_vfio.attach_dev(vm, params) | ||
| vm.start() | ||
| vm.wait_for_serial_login().close() |
There was a problem hiding this comment.
The if/else block for handling hotplug contains duplicated code for attaching devices and starting the VM. This can be refactored to be more concise and less repetitive by removing the redundant calls, which improves maintainability.
| if hotplug: | |
| vm.start() | |
| vm.wait_for_serial_login().close() | |
| dev_names = sriov_vfio.attach_dev(vm, params) | |
| else: | |
| dev_names = sriov_vfio.attach_dev(vm, params) | |
| vm.start() | |
| vm.wait_for_serial_login().close() | |
| if hotplug: | |
| vm.start() | |
| vm.wait_for_serial_login().close() | |
| dev_names = sriov_vfio.attach_dev(vm, params) | |
| if not hotplug: | |
| vm.start() | |
| vm.wait_for_serial_login().close() |
| session = vm.wait_for_serial_login( | ||
| timeout=int(params.get('login_timeout'))) |
There was a problem hiding this comment.
The login_timeout parameter is fetched from params and converted to an integer multiple times in this function (here, and on lines 81-82, 89-90). It would be more efficient and readable to do this once at the beginning of the run function and store the result in a variable.
For example:
# At the start of the run function
login_timeout = int(params.get('login_timeout', '240')) # Use a sensible default
# ... later in the code
session = vm.wait_for_serial_login(timeout=login_timeout)This avoids redundant calls and conversions, and centralizes the logic for getting the timeout.
|
/gemini summary |
Summary of ChangesThis pull request introduces a new automated test case focused on validating the lifecycle management of virtual machines that utilize SR-IOV VFIO devices. The test aims to ensure the stability and correct behavior of VMs through various operational states, from initial startup to shutdown and reboot, while managing the attached SR-IOV devices. Highlights
Changelog
Activity
|
|
/gemini summary |
Summary of ChangesThis pull request introduces a new automated test case designed to validate the comprehensive lifecycle management of virtual machines configured with SR-IOV VFIO devices. The test ensures the stability and correct behavior of VMs through various operational states, from initial startup to shutdown and reboot, while properly managing the attached SR-IOV devices. Highlights
Changelog
Activity
|
2 participants
No description provided.