Skip to content
Draft
Show file tree
Hide file tree
Changes from 9 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
32 changes: 31 additions & 1 deletion libvirt/tests/cfg/sriov/vIOMMU/iommu_device_settings.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
only q35
iface_model = 'rtl8139'
iface_dict = {'type_name': 'network', 'model': '${iface_model}', 'source': {'network': 'default'}}
- virtio_muti_devices:
- virtio_multi_devices:
disk_dict = {'target': {'dev': 'vda', 'bus': 'virtio'}, 'device': 'disk', 'driver': ${disk_driver}}
video_dict = {'primary': 'yes', 'model_heads': '1', 'model_type': 'virtio', 'driver': {'iommu': 'on'}}
test_devices = ["Eth", "block", "gpu"]
Expand Down Expand Up @@ -83,3 +83,33 @@
downstream_port = {'type': 'pci', 'model': 'pcie-switch-downstream-port', 'pre_controller': 'pcie-switch-upstream-port'}
root_port = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
controller_dicts = [{'type': 'pci', 'model': 'pcie-expander-bus', 'target': {'busNr': '250'}, 'pre_controller': 'pcie-root'}, ${root_port}, ${upstream_port}, ${upstream_port}, ${downstream_port}, ${downstream_port}]
- multiple_vfio_pcie_root_port:
only virtio
need_sriov = yes
ping_dest = ''
test_devices = ["Eth"]
# Multiple VFIO interfaces under pcie-root-port
# pcie-root <-- pcie-root-port <--- disk with iommu=on
# <-- pcie-root-port <- vfio interface
# <-- pcie-root-port <- vfio interface
controller_dicts = [{'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-root'}, {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-root'}, {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-root'}]
disk_dict = {'target': {'dev': 'vda', 'bus': 'virtio'}, 'device': 'disk', 'driver': ${disk_driver}}
iface_dict = [{'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr}}, {'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr2}}]
- multiple_vfio_pcie_expander_bus:
only virtio
need_sriov = yes
ping_dest = ''
test_devices = ["Eth"]
# Multiple VFIO interfaces with pcie-expander-bus
# pcie-root <--- pcie-expander-bus <-- pcie-root-port <--- virtio interface with iommu=on
# I |<----------------- pcie-root-port <--- vfio interface
# | |<------------------pcie-root-port <--- vfio interface
# | <----------pcie-root-port < ---virtio disk with iommu=on
expander_bus = {'type': 'pci', 'model': 'pcie-expander-bus', 'target': {'busNr': '251'}, 'pre_controller': 'pcie-root'}
root_port1 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
root_port2 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
root_port3 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
controller_dicts = [${expander_bus}, ${root_port1}, ${root_port2}, ${root_port3}]
disk_dict = {'target': {'dev': 'vda', 'bus': 'virtio'}, 'device': 'disk', 'driver': ${disk_driver}}
# Mixed interfaces: one virtio network with iommu=on and two VFIO interfaces
iface_dict = [{'type_name': 'network', 'model': 'virtio', 'driver': {'driver_attr': {'iommu': 'on'}}, 'source': {'network': 'default'}}, {'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr}}, {'managed': 'yes', 'type_name': 'hostdev', 'hostdev_address': {'type_name': 'pci', 'attrs': vf_pci_addr2}}]
24 changes: 21 additions & 3 deletions libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from virttest.libvirt_xml import vm_xml
from virttest.utils_libvirt import libvirt_vmxml
from virttest.utils_test import libvirt

from provider.sriov import sriov_base
from provider.viommu import viommu_base
Expand Down Expand Up @@ -39,6 +40,7 @@ def run(test, params, env):
vm_session = vm.wait_for_serial_login(
timeout=int(params.get('login_timeout')))
pre_devices = viommu_base.get_devices_pci(vm_session, test_devices)
vm_session.close()
vm.destroy()

for dev in ["disk", "video"]:
Expand All @@ -58,12 +60,24 @@ def run(test, params, env):
iface_dict = test_obj.parse_iface_dict()

if cleanup_ifaces:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", iface_dict)
# Handle both single dict and list of dicts
if isinstance(iface_dict, list):
libvirt_vmxml.remove_vm_devices_by_type(vm, 'interface')
for single_iface_dict in iface_dict:
dev_obj = libvirt_vmxml.create_vm_device_by_type("interface", single_iface_dict)
test.log.debug(f"XML of interface device is:\n{dev_obj}")
libvirt.add_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
dev_obj)
else:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", iface_dict)

test.log.info("TEST_STEP: Start the VM.")
vm.start()
vm.cleanup_serial_console()
vm.create_serial_console()
vm_session = vm.wait_for_serial_login(
timeout=int(params.get('login_timeout')))
test.log.debug(vm_xml.VMXML.new_from_dumpxml(vm.name))
Expand All @@ -87,4 +101,8 @@ def run(test, params, env):
if s:
test.fail("Failed to ping %s! status: %s, output: %s." % (ping_dest, s, o))
finally:
if 'vm_session' in locals():
test.log.debug("Closing vm_session")
vm_session.close()
vm.cleanup_serial_console()
test_obj.teardown_iommu_test()
151 changes: 134 additions & 17 deletions provider/viommu/viommu_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,35 @@ def __init__(self, vm, test, params, session=None):
libvirt_version.is_libvirt_feature_supported(self.params)
new_xml = vm_xml.VMXML.new_from_inactive_dumpxml(self.vm.name)
self.orig_config_xml = new_xml.copy()
self.test.log.debug("Original VM configuration backed up for restore")

def parse_iface_dict(self):
"""
Parse iface_dict from params
Parse iface_dict from params, supporting both single dict and list of dicts

:return: The updated iface_dict
:return: The updated iface_dict or list of iface_dicts based on type of input
"""
# generate mac address, if it is needed in iface_dict
mac_addr = utils_net.generate_mac_address_simple()
iface_dict = eval(self.params.get("iface_dict", "{}"))

if self.controller_dicts and iface_dict:
if isinstance(iface_dict, list):
result = []
for single_iface_dict in iface_dict:
self._update_iface_dict_with_controller_info(single_iface_dict)
result.append(single_iface_dict)
return result
else:
self._update_iface_dict_with_controller_info(iface_dict)
return iface_dict
Comment on lines +123 to +131
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 input validation and error handling.

The code assumes iface_dict is either a list or a dict but doesn't validate this. If parsing yields an unexpected type (e.g., string, int, None with truthy check bypassed), subsequent operations may fail with unclear errors.

Consider adding validation:

     iface_dict = literal_eval(self.params.get("iface_dict", "{}"))
+    
+    if not isinstance(iface_dict, (dict, list)):
+        raise ValueError(f"iface_dict must be dict or list, got {type(iface_dict).__name__}")

     if isinstance(iface_dict, list):
+        if not all(isinstance(item, dict) for item in iface_dict):
+            raise ValueError("All items in iface_dict list must be dicts")
         result = []
         for single_iface_dict in iface_dict:
             self._update_iface_dict_with_controller_info(single_iface_dict)
             result.append(single_iface_dict)
         return result
     else:
         self._update_iface_dict_with_controller_info(iface_dict)
         return iface_dict
📝 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 isinstance(iface_dict, list):
result = []
for single_iface_dict in iface_dict:
self._update_iface_dict_with_controller_info(single_iface_dict)
result.append(single_iface_dict)
return result
else:
self._update_iface_dict_with_controller_info(iface_dict)
return iface_dict
iface_dict = literal_eval(self.params.get("iface_dict", "{}"))
if not isinstance(iface_dict, (dict, list)):
raise ValueError(f"iface_dict must be dict or list, got {type(iface_dict).__name__}")
if isinstance(iface_dict, list):
if not all(isinstance(item, dict) for item in iface_dict):
raise ValueError("All items in iface_dict list must be dicts")
result = []
for single_iface_dict in iface_dict:
self._update_iface_dict_with_controller_info(single_iface_dict)
result.append(single_iface_dict)
return result
else:
self._update_iface_dict_with_controller_info(iface_dict)
return iface_dict


def _update_iface_dict_with_controller_info(self, iface_dict):
"""
Update interface dictionary with controller addressing information

:param iface_dict: Interface dictionary to update with controller info
"""
if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
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

Fix conditional check for empty dict.

The condition iface_dict will evaluate to False for an empty dict {}, potentially skipping valid updates. If an empty dict is intentionally meant to skip processing, this should be explicit.

Apply this diff to be explicit:

-    if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
+    if self.controller_dicts and iface_dict is not None and iface_dict.get('type_name') != 'hostdev':

Or, if empty dicts should skip processing:

-    if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
+    if self.controller_dicts and iface_dict and len(iface_dict) > 0 and iface_dict.get('type_name') != 'hostdev':
📝 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 self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev':
if self.controller_dicts and iface_dict is not None and iface_dict.get('type_name') != 'hostdev':
🤖 Prompt for AI Agents
provider/viommu/viommu_base.py around line 139: the current conditional relies
on the truthiness of iface_dict so an empty dict will be treated as false and
unintentionally skipped; replace the truthiness check with an explicit test—if
empty dicts should be processed, change the condition to explicitly check that
iface_dict is not None before checking its type_name; if empty dicts should be
skipped, make that explicit by testing that iface_dict is not an empty dict (or
explicitly checking for existence of type_name) before comparing to 'hostdev'.

iface_bus = "%0#4x" % int(self.controller_dicts[-1].get("index"))
iface_attrs = {"bus": iface_bus}
if self.controller_dicts[-1].get("model") == "pcie-to-pci-bridge":
Expand All @@ -129,7 +147,6 @@ def parse_iface_dict(self):

iface_dict.update({"address": {"attrs": iface_attrs}})
self.test.log.debug("iface_dict: %s.", iface_dict)
return iface_dict

def prepare_controller(self):
"""
Expand All @@ -143,17 +160,11 @@ def prepare_controller(self):
contr_dict = self.controller_dicts[i]
pre_controller = contr_dict.get("pre_controller")
if pre_controller:
pre_contrs = list(
filter(None, [c.get("index") for c in self.controller_dicts
if c["type"] == contr_dict["type"] and
c["model"] == pre_controller]))
if pre_contrs:
pre_idx = pre_contrs[0]
else:
pre_idx = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], pre_controller)[-1]
self.test.log.debug(f"Processing controller {i}: {contr_dict}")
self.test.log.debug(f"Looking for pre_controller: {pre_controller}")
pre_idx = self._find_pre_controller_index(pre_controller, contr_dict)
pre_contr_bus = "%0#4x" % int(pre_idx)
self.test.log.debug(f"Parent controller index: {pre_idx}, bus: {pre_contr_bus}")
contr_attrs = {"bus": pre_contr_bus}
contr_slot = libvirt_pcicontr.get_free_pci_slot(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
Expand All @@ -178,12 +189,24 @@ def prepare_controller(self):

contr_dict.update({"address": {"attrs": contr_attrs}})
contr_dict.pop("pre_controller")
self.test.log.debug(f"Final controller config: {contr_dict}")
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(self.vm.name), "controller",
contr_dict, 100)
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
# Only assign index if it wasn't already assigned
if "index" not in contr_dict:
# Check if target busNr is specified and use it as index
if contr_dict.get("target", {}).get("busNr"):
target_bus = int(contr_dict["target"]["busNr"])
contr_dict["index"] = target_bus
self.test.log.debug(f"Using target busNr {target_bus} as index for controller {i}: {contr_dict['model']}")
else:
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
self.test.log.debug(f"Assigned index {contr_dict['index']} to controller {i}: {contr_dict['model']}")
else:
self.test.log.debug(f"Index already assigned {contr_dict['index']} for controller {i}: {contr_dict['model']}")
Comment on lines 282 to 289
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

Fix truthy check that incorrectly rejects busNr=0.

Line 199 uses a truthy check on busNr, which evaluates to False when busNr is 0. Since 0 is a valid bus number, this check incorrectly skips the target busNr logic and falls through to the else branch.

Apply this diff to check explicitly for None:

-            if contr_dict.get("target", {}).get("busNr"):
+            if contr_dict.get("target", {}).get("busNr") is not None:
                 target_bus = int(contr_dict["target"]["busNr"])
📝 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
# Only assign index if it wasn't already assigned
if "index" not in contr_dict:
# Check if target busNr is specified and use it as index
if contr_dict.get("target", {}).get("busNr"):
target_bus = int(contr_dict["target"]["busNr"])
contr_dict["index"] = target_bus
self.test.log.debug(f"Using target busNr {target_bus} as index for controller {i}: {contr_dict['model']}")
else:
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
self.test.log.debug(f"Assigned index {contr_dict['index']} to controller {i}: {contr_dict['model']}")
else:
self.test.log.debug(f"Index already assigned {contr_dict['index']} for controller {i}: {contr_dict['model']}")
# Only assign index if it wasn't already assigned
if "index" not in contr_dict:
# Check if target busNr is specified and use it as index
if contr_dict.get("target", {}).get("busNr") is not None:
target_bus = int(contr_dict["target"]["busNr"])
contr_dict["index"] = target_bus
self.test.log.debug(f"Using target busNr {target_bus} as index for controller {i}: {contr_dict['model']}")
else:
contr_dict["index"] = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], contr_dict["model"])[-1]
self.test.log.debug(f"Assigned index {contr_dict['index']} to controller {i}: {contr_dict['model']}")
else:
self.test.log.debug(f"Index already assigned {contr_dict['index']} for controller {i}: {contr_dict['model']}")
🤖 Prompt for AI Agents
In provider/viommu/viommu_base.py around lines 196 to 209, the truthy check on
contr_dict.get("target", {}).get("busNr") incorrectly treats busNr==0 as absent;
change the logic to explicitly test for None (e.g., bus_nr =
contr_dict.get("target", {}).get("busNr"); if bus_nr is not None: target_bus =
int(bus_nr); contr_dict["index"] = target_bus; ...) so zero is accepted while
still handling missing values, and keep the debug log and remaining branches
unchanged.

self.controller_dicts[i] = contr_dict

self.test.log.debug(f"Prepared controllers according to {self.controller_dicts}")
Expand Down Expand Up @@ -243,3 +266,97 @@ def teardown_iommu_test(self, **dargs):
if self.vm.is_alive():
self.vm.destroy(gracefully=False)
self.orig_config_xml.sync()
self.test.log.debug("VM configuration restored to original state")

def _find_pre_controller_index(self, pre_controller, contr_dict):
"""
Find the index of the pre_controller using flexible lookup methods.

:param pre_controller: The pre_controller value to look up
:param contr_dict: The current controller dictionary
:return: The index of the pre_controller
"""
self.test.log.debug(f"Finding pre_controller: {pre_controller}")

result = None
# Try various dictionary format first (most explicit)
if isinstance(pre_controller, dict):
result = self._find_by_dict_format(pre_controller)

# Try direct index (numeric string)
if result is None and isinstance(pre_controller, str) and pre_controller.isdigit():
result = self._find_by_direct_index(pre_controller)

# Try model name (backward compatibility)
if result is None and isinstance(pre_controller, str):
result = self._find_by_model_name(pre_controller, contr_dict)

# Fallback to existing VM controllers
if result is None:
self.test.log.debug(f"Fallback: Looking for existing controller with model {pre_controller}")
result = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], pre_controller)[-1]
self.test.log.debug(f"Using fallback index: {result}")

return result
Comment on lines +355 to +386
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Validate non-empty list before accessing [-1] on fallback.

Line 299 accesses [-1] on the result of get_max_contr_indexes() without verifying the list is non-empty. If no matching controllers exist, this raises IndexError.

Apply this diff to add validation:

         # Fallback to existing VM controllers
         if result is None:
             self.test.log.debug(f"Fallback: Looking for existing controller with model {pre_controller}")
-            result = libvirt_pcicontr.get_max_contr_indexes(
+            indexes = libvirt_pcicontr.get_max_contr_indexes(
                 vm_xml.VMXML.new_from_dumpxml(self.vm.name),
-                contr_dict["type"], pre_controller)[-1]
+                contr_dict["type"], pre_controller)
+            if not indexes:
+                raise ValueError(f"No controllers found matching type={contr_dict['type']}, model={pre_controller}")
+            result = indexes[-1]
             self.test.log.debug(f"Using fallback index: {result}")
📝 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
def _find_pre_controller_index(self, pre_controller, contr_dict):
"""
Find the index of the pre_controller using flexible lookup methods.
:param pre_controller: The pre_controller value to look up
:param contr_dict: The current controller dictionary
:return: The index of the pre_controller
"""
self.test.log.debug(f"Finding pre_controller: {pre_controller}")
result = None
# Try various dictionary format first (most explicit)
if isinstance(pre_controller, dict):
result = self._find_by_dict_format(pre_controller)
# Try direct index (numeric string)
if result is None and isinstance(pre_controller, str) and pre_controller.isdigit():
result = self._find_by_direct_index(pre_controller)
# Try model name (backward compatibility)
if result is None and isinstance(pre_controller, str):
result = self._find_by_model_name(pre_controller, contr_dict)
# Fallback to existing VM controllers
if result is None:
self.test.log.debug(f"Fallback: Looking for existing controller with model {pre_controller}")
result = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], pre_controller)[-1]
self.test.log.debug(f"Using fallback index: {result}")
return result
# Fallback to existing VM controllers
if result is None:
self.test.log.debug(f"Fallback: Looking for existing controller with model {pre_controller}")
indexes = libvirt_pcicontr.get_max_contr_indexes(
vm_xml.VMXML.new_from_dumpxml(self.vm.name),
contr_dict["type"], pre_controller)
if not indexes:
raise ValueError(f"No controllers found matching type={contr_dict['type']}, model={pre_controller}")
result = indexes[-1]
self.test.log.debug(f"Using fallback index: {result}")
🤖 Prompt for AI Agents
In provider/viommu/viommu_base.py around lines 271 to 302, the fallback path
calls libvirt_pcicontr.get_max_contr_indexes(...)[-1] without verifying the
returned list is non-empty which can raise IndexError; change this to first
assign the result to a variable, check if the list is truthy (non-empty), and
only then take the last element; if the list is empty, log a debug/warning with
context (vm name, contr_dict["type"], pre_controller) and set result to None (or
return None) so the caller safely handles the missing controller index.


def _controller_exists_by_index(self, index):
"""Check if a controller with the given index exists in controller_dicts"""
for c in self.controller_dicts:
if c.get("index") == index:
return True
return False

def _find_by_dict_format(self, pre_controller):
"""
Find controller by dictionary format: {'alias': 'name'} or {'index': N}
example: {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': {'index': 100,'alias': 'pci.100'}}
"""
if 'index' in pre_controller:
pre_idx = int(pre_controller['index'])
# Validate that controller exists in our controller_dicts
if self._controller_exists_by_index(pre_idx):
self.test.log.debug(f"Using explicit index from dict: {pre_idx}")
return pre_idx
elif 'alias' in pre_controller:
alias_name = pre_controller['alias']
return self._find_by_alias_name(alias_name)
self.test.log.warning(f"Invalid pre_controller dict format: {pre_controller}, or controller not found in controller_dicts. Use {{'index': N}} or {{'alias': 'name'}}, will fall back to other methods")
return None
Comment on lines +395 to +410
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 error handling for int() conversion.

Line 317 converts pre_controller['index'] to int without validation. If the value is not a valid integer, this raises ValueError.

Apply this diff to add error handling:

         if 'index' in pre_controller:
-            pre_idx = int(pre_controller['index'])
+            try:
+                pre_idx = int(pre_controller['index'])
+            except (ValueError, TypeError) as e:
+                self.test.log.warning(f"Invalid index value in pre_controller dict: {pre_controller['index']}: {e}")
+                return None
             # Validate that controller exists in our controller_dicts
📝 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
def _find_by_dict_format(self, pre_controller):
"""
Find controller by dictionary format: {'alias': 'name'} or {'index': N}
example: {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': {'index': 100,'alias': 'pci.100'}}
"""
if 'index' in pre_controller:
pre_idx = int(pre_controller['index'])
# Validate that controller exists in our controller_dicts
if self._controller_exists_by_index(pre_idx):
self.test.log.debug(f"Using explicit index from dict: {pre_idx}")
return pre_idx
elif 'alias' in pre_controller:
alias_name = pre_controller['alias']
return self._find_by_alias_name(alias_name)
self.test.log.warning(f"Invalid pre_controller dict format: {pre_controller}, or controller not found in controller_dicts. Use {{'index': N}} or {{'alias': 'name'}}, will fall back to other methods")
return None
def _find_by_dict_format(self, pre_controller):
"""
Find controller by dictionary format: {'alias': 'name'} or {'index': N}
example: {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': {'index': 100,'alias': 'pci.100'}}
"""
if 'index' in pre_controller:
try:
pre_idx = int(pre_controller['index'])
except (ValueError, TypeError) as e:
self.test.log.warning(f"Invalid index value in pre_controller dict: {pre_controller['index']}: {e}")
return None
# Validate that controller exists in our controller_dicts
if self._controller_exists_by_index(pre_idx):
self.test.log.debug(f"Using explicit index from dict: {pre_idx}")
return pre_idx
elif 'alias' in pre_controller:
alias_name = pre_controller['alias']
return self._find_by_alias_name(alias_name)
self.test.log.warning(
f"Invalid pre_controller dict format: {pre_controller}, "
"or controller not found in controller_dicts. "
"Use {'index': N} or {'alias': 'name'}, will fall back to other methods"
)
return None
🤖 Prompt for AI Agents
In provider/viommu/viommu_base.py around lines 311 to 326, the code calls
int(pre_controller['index']) without validation which can raise ValueError (or
TypeError) for non-integer inputs; wrap the conversion in a try/except catching
ValueError and TypeError, log a clear error message including the invalid value
and pre_controller dict (use self.test.log.error or warning), and return None so
the function falls back to other lookup methods instead of raising.


def _find_by_direct_index(self, pre_controller):
"""
Find controller by direct index (numeric string) direct index passed in configuration
example: {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': '100'}
"""
pre_idx = int(pre_controller)
# Validate that controller exists in our controller_dicts
if self._controller_exists_by_index(pre_idx):
self.test.log.debug(f"Using direct index: {pre_idx}")
return pre_idx
else:
self.test.log.warning(f"Controller with index {pre_idx} not found in controller_dicts, will fall back to other methods")
return None

def _find_by_alias_name(self, alias_name):
"""Find controller by alias name passed in configuration """
for c in self.controller_dicts:
if c.get("alias", {}).get("name") == alias_name:
pre_idx = int(c.get("index"))
self.test.log.debug(f"Found controller by alias {alias_name}: index {pre_idx}")
return pre_idx
self.test.log.warning(f"Controller with alias '{alias_name}' not found in controller_dicts, will fall back to other methods")
return None
Comment on lines +426 to +434
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 before int() conversion.

Line 346 converts c.get("index") to int without checking if it's None. If a controller's index is missing, this raises TypeError.

Apply this diff to add validation:

         for c in self.controller_dicts:
             if c.get("alias", {}).get("name") == alias_name:
-                pre_idx = int(c.get("index"))
+                index = c.get("index")
+                if index is None:
+                    self.test.log.warning(f"Controller with alias '{alias_name}' has no index")
+                    continue
+                pre_idx = int(index)
                 self.test.log.debug(f"Found controller by alias {alias_name}: index {pre_idx}")
📝 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
def _find_by_alias_name(self, alias_name):
"""Find controller by alias name passed in configuration """
for c in self.controller_dicts:
if c.get("alias", {}).get("name") == alias_name:
pre_idx = int(c.get("index"))
self.test.log.debug(f"Found controller by alias {alias_name}: index {pre_idx}")
return pre_idx
self.test.log.warning(f"Controller with alias '{alias_name}' not found in controller_dicts, will fall back to other methods")
return None
def _find_by_alias_name(self, alias_name):
"""Find controller by alias name passed in configuration """
for c in self.controller_dicts:
if c.get("alias", {}).get("name") == alias_name:
index = c.get("index")
if index is None:
self.test.log.warning(f"Controller with alias '{alias_name}' has no index")
continue
pre_idx = int(index)
self.test.log.debug(f"Found controller by alias {alias_name}: index {pre_idx}")
return pre_idx
self.test.log.warning(f"Controller with alias '{alias_name}' not found in controller_dicts, will fall back to other methods")
return None
🤖 Prompt for AI Agents
In provider/viommu/viommu_base.py around lines 342 to 350, the code calls
int(c.get("index")) without validating that c.get("index") is present and
convertible, which can raise TypeError/ValueError; update the loop to retrieve
index_raw = c.get("index"), check if index_raw is not None (and optionally try
to convert with try/except ValueError), log a warning and continue to next
controller if missing/invalid, and only set pre_idx = int(index_raw) and return
it when conversion succeeds.


def _find_by_model_name(self, pre_controller, contr_dict):
"""Find controller by model name (backward compatibility)"""
pre_contrs = list(
filter(None, [c.get("index") for c in self.controller_dicts
if c["type"] == contr_dict["type"] and
c["model"] == pre_controller]))
if pre_contrs:
pre_idx = pre_contrs[0]
self.test.log.debug(f"Found controller by model {pre_controller}: index {pre_idx}")
return pre_idx
return None