Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions libvirt/tests/cfg/virtual_disks/virtual_disks_product.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
- virtual_disks.product_info:
type = virtual_disks_product
start_vm = no
target_disk = "sda"
disk_bus = "sata"
disk_type = "file"
status_error = "no"
variants:
- sata_bus:
func_supported_since_libvirt_ver = (11, 1, 0)
disk_bus = "sata"
variants:
- positive_test:
variants:
- length_less_max:
disk_product = "20BE0061MC20BE0061MC"
- length_equal_max:
disk_product = "20BE0061MC20BE0061MC20BE0061MC20BE0061MC"
- negative_test:
variants:
- length_more_max:
status_error = "yes"
disk_product = "20BE0061MC20BE0061MC20BE0061MC20BE0061MC1"
expected_error = "disk product is more than 40 characters"
disk_dict = {"type_name":"${disk_type}", "target":{"dev": "${target_disk}", "bus": "${disk_bus}"}, "driver": {"name": "qemu", "type": "qcow2"}, "product": "${disk_product}"}
77 changes: 77 additions & 0 deletions libvirt/tests/src/virtual_disks/virtual_disks_product.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
from ast import literal_eval

from virttest import libvirt_version
from virttest import virsh
from virttest.libvirt_xml import vm_xml, xcepts
from virttest.utils_libvirt import libvirt_disk

from provider.virtual_disk import disk_base


def run(test, params, env):
"""
Test disk with product info.

1. Define a guest with a disk include product info.
2. Start guest.
3. Login guest, check the disk info.
4. Confirm that the product info is same as xml setting.
"""
def prepare_disk():
"""
Prepare the disk with product info.
"""
disk_dict = literal_eval(params.get("disk_dict", "{}"))
expected_error = params.get("expected_error")
status_error = "yes" == params.get("status_error", "no")
try:
disk_obj.add_vm_disk(disk_type, disk_dict)
except xcepts.LibvirtXMLError as xml_error:
if not status_error:
test.fail(f"Failed to define VM:\n {str(xml_error)}")
else:
if expected_error and expected_error not in str(xml_error):
test.fail(f"Expected error '{expected_error}' not found in"
" actual error: {xml_error}")
test.log.debug(f"Get expected error message:\n {expected_error}")
return False
return True

def check_guest():
"""
Check the disk info in guest.
"""
vm_session = vm.wait_for_login()
new_disk, _ = libvirt_disk.get_non_root_disk_name(vm_session)
sg_command = "sg_inq -p di -v /dev/%s" % new_disk
sg_output = vm_session.cmd_output(sg_command)
if disk_product not in sg_output:
test.fail(f"Product info '{disk_product}' not found in command output: {sg_output}")
Comment on lines +46 to +49
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add validation for disk_product parameter.

The code uses disk_product without checking if it's None. If the parameter is missing from the test configuration, line 48 will raise a TypeError when attempting the in operation.

Apply this diff to add validation:

     def check_guest():
         """
         Check the disk info in guest.
         """
+        if not disk_product:
+            test.error("disk_product parameter is required but not provided")
         vm_session = vm.wait_for_login()
         new_disk, _ = libvirt_disk.get_non_root_disk_name(vm_session)
         sg_command = "sg_inq -p di -v /dev/%s" % new_disk
         sg_output = vm_session.cmd_output(sg_command)
         if disk_product not in sg_output:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sg_command = "sg_inq -p di -v /dev/%s" % new_disk
sg_output = vm_session.cmd_output(sg_command)
if disk_product not in sg_output:
test.fail(f"Product info '{disk_product}' not found in command output: {sg_output}")
def check_guest():
"""
Check the disk info in guest.
"""
if not disk_product:
test.error("disk_product parameter is required but not provided")
vm_session = vm.wait_for_login()
new_disk, _ = libvirt_disk.get_non_root_disk_name(vm_session)
sg_command = "sg_inq -p di -v /dev/%s" % new_disk
sg_output = vm_session.cmd_output(sg_command)
if disk_product not in sg_output:
test.fail(f"Product info '{disk_product}' not found in command output: {sg_output}")
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_disks/virtual_disks_product.py around lines 46 to
49, the code uses disk_product without validating it, causing a TypeError if
it's None; add an explicit check right before running sg_inq: if disk_product is
None (or falsy), call test.fail with a clear message (or raise an appropriate
test skip/error), otherwise proceed to run sg_inq and check membership as before
so the in operation is safe.

else:
test.log.debug("Product info found in guest as expected.")

libvirt_version.is_libvirt_feature_supported(params)
vm_name = params.get("main_vm")
disk_type = params.get("disk_type", "file")
disk_product = params.get("disk_product")
vm = env.get_vm(vm_name)

vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
backup_xml = vmxml.copy()
disk_obj = disk_base.DiskBase(test, vm, params)

try:
test.log.info("Start a guest with product info.")
if not prepare_disk():
return
if not vm.is_alive():
vm.start()
Comment on lines +67 to +68
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Add error handling for VM start.

The VM start operation doesn't handle potential failures. If vm.start() fails, the test should provide a clear failure message rather than proceeding to the guest check.

Apply this diff:

         if not vm.is_alive():
-            vm.start()
+            try:
+                vm.start()
+            except Exception as e:
+                test.fail(f"Failed to start VM: {e}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if not vm.is_alive():
vm.start()
if not vm.is_alive():
try:
vm.start()
except Exception as e:
test.fail(f"Failed to start VM: {e}")
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_disks/virtual_disks_product.py around lines 62-63,
the call to vm.start() lacks error handling; wrap the start operation in a
try/except (or check its return status) and, on failure, make the test fail
immediately with a clear message (e.g., use pytest.fail or self.fail depending
on the test framework) including VM identifier and the caught exception; also
consider verifying vm.is_alive() after a successful start and failing with a
clear message if it is not alive.

test.log.debug(f"The current guest xml is: {virsh.dumpxml(vm_name).stdout_text}")
test.log.info("Check the product info in guest.")
check_guest()

finally:
if vm.is_alive():
vm.destroy()
backup_xml.sync()
disk_obj.cleanup_disk_preparation(disk_type)
Comment on lines +73 to +77
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Ensure cleanup completes even if VM destruction fails.

The current cleanup logic could leave resources if vm.destroy() raises an exception, preventing backup_xml.sync() and cleanup_disk_preparation() from executing. Use nested try-except blocks to ensure all cleanup steps attempt to complete.

Apply this diff:

     finally:
-        if vm.is_alive():
-            vm.destroy()
-        backup_xml.sync()
-        disk_obj.cleanup_disk_preparation(disk_type)
+        try:
+            if vm.is_alive():
+                vm.destroy()
+        finally:
+            backup_xml.sync()
+            disk_obj.cleanup_disk_preparation(disk_type)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
finally:
if vm.is_alive():
vm.destroy()
backup_xml.sync()
disk_obj.cleanup_disk_preparation(disk_type)
finally:
try:
if vm.is_alive():
vm.destroy()
finally:
backup_xml.sync()
disk_obj.cleanup_disk_preparation(disk_type)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_disks/virtual_disks_product.py around lines 68 to
72, the finally block currently calls vm.destroy(), backup_xml.sync(), and
disk_obj.cleanup_disk_preparation(disk_type) sequentially so an exception from
vm.destroy() will short-circuit the remaining cleanup; wrap vm.destroy() in its
own try-except (log the exception but do not re-raise), then unconditionally
call backup_xml.sync() and disk_obj.cleanup_disk_preparation(disk_type) in the
outer finally so all cleanup steps are attempted even if VM destruction fails.