LIBVIRTAT-20967: Add multiple VFIO interfaces in vIOMMU test #6590
Conversation
…ests - Enhanced VIOMMUTest.parse_iface_dict() to handle both single dict and list of dicts - Added _update_iface_dict_with_controller_info() helper method for maintainability - Updated iommu_device_settings.py to handle multiple interfaces in modify_vm_device() - Added two new test configurations for multiple VFIO interfaces: * Case 10: Multiple VFIO interfaces under pcie-root-port * Case 11: Multiple VFIO interfaces with pcie-expander-bus - Maintained backward compatibility with existing single interface configurations - All changes pass inspekt code quality checks
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRenames a config variant key and adds two multi‑VFIO topology variants; updates test logic to close VM session earlier, handle single-or-multiple iface dicts and manage serial consoles; enhances provider parsing to attach controller-derived PCI addressing and flexible pre-controller resolution. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test (iface cleanup)
participant Provider as VIOMMUTest.parse_iface_dict
participant PCI as libvirt_pcicontr
participant Controller as Controller resolution helpers
rect rgba(230,240,255,0.6)
note over Test,Provider: Input can be a single iface dict or a list
Test->>Provider: parse_iface_dict(iface_dict(s), controller_dicts)
Provider->>Controller: _find_pre_controller_index(pre_controller, contr_dict)
alt pre_controller resolved by dict/index/name
Controller-->>Provider: index (bus/slot base)
else fallback to max/index
Controller-->>Provider: computed index
end
alt iface list
loop per iface
Provider->>PCI: get_free_pci_slot (if model == "pcie-to-pci-bridge")
PCI-->>Provider: slot (optional)
Provider-->>Test: updated iface (with address)
end
else single iface
Provider->>PCI: get_free_pci_slot (conditional)
PCI-->>Provider: slot (optional)
Provider-->>Test: updated iface (with address)
end
end
rect rgba(240,255,240,0.6)
note over Test: Remove existing iface(s) then add each updated iface
loop per updated iface (or single)
Test->>Test: remove existing iface (if present)
Test->>Test: create/add updated iface (modify_vm_device)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
provider/viommu/viommu_base.py (1)
106-106: Replaceeval()withast.literal_eval()for security.Similar to line 120, using
eval()here poses a code injection risk. For consistency and security, replace withast.literal_eval().Apply this diff:
+from ast import literal_eval + def __init__(self, vm, test, params, session=None): self.vm = vm self.test = test self.params = params self.session = session self.remote_virsh_dargs = None - self.controller_dicts = eval(self.params.get("controller_dicts", "[]")) + self.controller_dicts = literal_eval(self.params.get("controller_dicts", "[]"))
🧹 Nitpick comments (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py (1)
61-70: Optional: Reduce code duplication.The
modify_vm_devicecall is duplicated in both branches. Consider refactoring to handle both cases uniformly:if cleanup_ifaces: # Handle both single dict and list of dicts - if isinstance(iface_dict, list): - for single_iface_dict in iface_dict: - libvirt_vmxml.modify_vm_device( - vm_xml.VMXML.new_from_dumpxml(vm.name), - "interface", single_iface_dict) - else: - libvirt_vmxml.modify_vm_device( - vm_xml.VMXML.new_from_dumpxml(vm.name), - "interface", iface_dict) + iface_dicts_to_cleanup = iface_dict if isinstance(iface_dict, list) else [iface_dict] + for single_iface_dict in iface_dicts_to_cleanup: + libvirt_vmxml.modify_vm_device( + vm_xml.VMXML.new_from_dumpxml(vm.name), + "interface", single_iface_dict)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/sriov/vIOMMU/iommu_device_settings.cfg(2 hunks)libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py(1 hunks)provider/viommu/viommu_base.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.2)
provider/viommu/viommu_base.py
119-119: Local variable mac_addr is assigned to but never used
Remove assignment to unused variable mac_addr
(F841)
120-120: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
🔇 Additional comments (4)
provider/viommu/viommu_base.py (2)
132-148: LGTM!The helper method cleanly encapsulates controller addressing logic with appropriate special handling for
pcie-to-pci-bridgemodels. The in-place mutation ofiface_dictis consistent with the class's pattern and keeps the interface simple.
122-130: Audit parse_iface_dict callers for list return
Outside provider/sRIoV/iommu_device_settings.py, all callers treat the return value as a single dict (e.g., calling.get()or passing it directly tocreate_iface), which will break if a list is returned. Confirm inputs never produce a list or update callers to handle lists appropriately.libvirt/tests/cfg/sriov/vIOMMU/iommu_device_settings.cfg (2)
29-29: LGTM! Typo fix improves clarity.The variant name has been corrected from
virtio_muti_devicestovirtio_multi_devices, fixing the spelling error.
86-97: vf_pci_addr2 is properly defined in the test suite and available for use; no further action required.
| - 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': {'name': 'vhost', '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}}] |
There was a problem hiding this comment.
Verify controller count and device placement.
The ASCII diagram in lines 103-107 suggests 4 pcie-root-ports are needed (one each for: virtio network interface, two VFIO interfaces, and virtio disk), but controller_dicts at line 112 only defines 3 root-ports (root_port1, root_port2, root_port3).
Additionally, this variant introduces:
- Mixed interface types in
iface_dict(line 115): one virtio network and two VFIO hostdevs - Reference to
vf_pci_addr2for the second VFIO interface
Please clarify:
- Are 3 root-ports sufficient, or should a 4th be added for the disk?
- Confirm that
vf_pci_addr2is properly defined in the test environment - Verify that the test code (
parse_iface_dict()and related functions) correctly handles a list with mixed interface types (virtio network + hostdev)
Consider updating the comment or adding a 4th root-port to match the described topology:
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}]
+ root_port4 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'}
+ controller_dicts = [${expander_bus}, ${root_port1}, ${root_port2}, ${root_port3}, ${root_port4}]📝 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.
| - 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': {'name': 'vhost', '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}}] | |
| 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'} | |
| root_port4 = {'type': 'pci', 'model': 'pcie-root-port', 'pre_controller': 'pcie-expander-bus'} | |
| controller_dicts = [${expander_bus}, ${root_port1}, ${root_port2}, ${root_port3}, ${root_port4}] |
🤖 Prompt for AI Agents
libvirt/tests/cfg/sriov/vIOMMU/iommu_device_settings.cfg lines 98-115: the ASCII
topology implies four pcie-root-ports (virtio net, vfio1, vfio2, virtio disk)
but controller_dicts only includes three root_port entries; add a fourth
root_port definition (root_port4) and include it in controller_dicts so the disk
has its own root-port, ensure vf_pci_addr2 is declared in the test fixture or
config before use, and verify/adjust parse_iface_dict() and any related parsing
code to correctly accept a list with mixed interface dicts (handle 'network'
virtio entries and 'hostdev' pci entries in the same list), adding/adjusting
checks or unit tests to validate correct placement of each device on the
expected controllers.
| # Handle both single dict and list of dicts | ||
| if isinstance(iface_dict, list): | ||
| for single_iface_dict in iface_dict: | ||
| libvirt_vmxml.modify_vm_device( | ||
| vm_xml.VMXML.new_from_dumpxml(vm.name), | ||
| "interface", single_iface_dict) | ||
| else: | ||
| libvirt_vmxml.modify_vm_device( | ||
| vm_xml.VMXML.new_from_dumpxml(vm.name), | ||
| "interface", iface_dict) |
There was a problem hiding this comment.
🧩 Analysis chain
Consider error handling for multi-interface cleanup.
The current implementation iterates through interface dicts without error handling. If modify_vm_device fails on one interface, subsequent interfaces won't be cleaned up, potentially leaving the VM in an inconsistent state.
Consider wrapping the cleanup calls in a try-except block to ensure all interfaces are attempted:
if cleanup_ifaces:
# Handle both single dict and list of dicts
if isinstance(iface_dict, list):
for single_iface_dict in iface_dict:
- libvirt_vmxml.modify_vm_device(
- vm_xml.VMXML.new_from_dumpxml(vm.name),
- "interface", single_iface_dict)
+ try:
+ libvirt_vmxml.modify_vm_device(
+ vm_xml.VMXML.new_from_dumpxml(vm.name),
+ "interface", single_iface_dict)
+ except Exception as e:
+ test.log.warning(f"Failed to cleanup interface: {e}")
else:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", iface_dict)Additionally, verify that parse_iface_dict() validates list contents to prevent non-dict items from reaching this code.
🏁 Script executed:
#!/bin/bash
# Description: Verify parse_iface_dict() implementation and its validation logic
# Search for the parse_iface_dict method definition
ast-grep --pattern $'def parse_iface_dict($$$) {
$$$
}'
# Also search for type validation or list handling in parse_iface_dict
rg -n -C5 'def parse_iface_dict' --type=pyLength of output: 4071
🏁 Script executed:
#!/bin/bash
sed -n '110,200p' provider/viommu/viommu_base.pyLength of output: 4517
Isolate interface cleanup failures and validate types
Wrap each modify_vm_device call in a try-except so one failure doesn’t block subsequent cleanups, and skip or error on non-dict entries in the list, e.g.:
if cleanup_ifaces:
# Handle both single dict and list of dicts
if isinstance(iface_dict, list):
for single_iface_dict in iface_dict:
+ if not isinstance(single_iface_dict, dict):
+ test.log.warning(f"Skipping invalid iface entry: {single_iface_dict!r}")
+ continue
+ try:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", single_iface_dict)
+ except Exception as e:
+ test.log.warning(f"Failed to cleanup interface: {e}")
else:
libvirt_vmxml.modify_vm_device(
vm_xml.VMXML.new_from_dumpxml(vm.name),
"interface", 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.
| # Handle both single dict and list of dicts | |
| if isinstance(iface_dict, list): | |
| for single_iface_dict in iface_dict: | |
| libvirt_vmxml.modify_vm_device( | |
| vm_xml.VMXML.new_from_dumpxml(vm.name), | |
| "interface", single_iface_dict) | |
| else: | |
| 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): | |
| for single_iface_dict in iface_dict: | |
| if not isinstance(single_iface_dict, dict): | |
| test.log.warning(f"Skipping invalid iface entry: {single_iface_dict!r}") | |
| continue | |
| try: | |
| libvirt_vmxml.modify_vm_device( | |
| vm_xml.VMXML.new_from_dumpxml(vm.name), | |
| "interface", single_iface_dict) | |
| except Exception as e: | |
| test.log.warning(f"Failed to cleanup interface: {e}") | |
| else: | |
| libvirt_vmxml.modify_vm_device( | |
| vm_xml.VMXML.new_from_dumpxml(vm.name), | |
| "interface", iface_dict) |
🤖 Prompt for AI Agents
In libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py around lines 61 to
70, wrap each libvirt_vmxml.modify_vm_device call in a try/except so one failing
cleanup won’t stop others, and validate input types: when iterating a list,
check isinstance(single_iface_dict, dict) and skip/log a warning for non-dict
entries, and in the non-list branch ensure iface_dict is a dict before calling
modify_vm_device; on exception catch Exception as e, log the error with details
and continue to the next item.
…ification - Fixed VIOMMUTest to skip address attributes for hostdev interfaces to prevent conflicts - Added proper console session cleanup to prevent hanging sessions during debugging - Enhanced dmesg IOMMU verification to properly check for IOMMU group messages - Added debug logging for IOMMU group messages found - Improved error handling with clear failure messages for IOMMU verification - All changes maintain backward compatibility and pass code quality checks
- Use remove-then-add approach for multiple interfaces to ensure clean state - Add proper console session cleanup to prevent 'existing session' errors - Remove all interfaces before adding new ones to avoid conflicts - Use add_vm_device for multiple interfaces and modify_vm_device for single interface - Note: Single interface still uses modify_vm_device to maintain existing test logic - Consider: Future improvement could use consistent remove-then-add approach for all cases
- Use remove-then-add approach for multiple interfaces to ensure clean state - Add proper console session cleanup to prevent 'existing session' errors - Use correct libvirt.add_vm_device method from virttest.utils_test - Add consistent debug logging for interface device XML - Remove all interfaces before adding new ones to avoid conflicts - Note: Single interface still uses modify_vm_device to maintain existing test logic
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py (1)
62-75: Harden multi-interface cleanup: add per-item try/except and type validation.One failure can block subsequent interfaces; non-dict entries would also slip through. Handle both.
if cleanup_ifaces: # 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) + for single_iface_dict in iface_dict: + if not isinstance(single_iface_dict, dict): + test.log.warning(f"Skipping invalid iface entry: {single_iface_dict!r}") + continue + try: + 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) + except Exception as e: + test.log.warning(f"Failed to add interface {single_iface_dict!r}: {e}") else: - libvirt_vmxml.modify_vm_device( - vm_xml.VMXML.new_from_dumpxml(vm.name), - "interface", iface_dict) + if not isinstance(iface_dict, dict): + test.log.warning(f"Skipping invalid iface_dict: {iface_dict!r}") + else: + try: + libvirt_vmxml.modify_vm_device( + vm_xml.VMXML.new_from_dumpxml(vm.name), + "interface", iface_dict) + except Exception as e: + test.log.warning(f"Failed to modify interface {iface_dict!r}: {e}")Optional: fetch VMXML once before the loop and reuse to avoid repeated new_from_dumpxml calls.
🧹 Nitpick comments (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py (1)
6-6: Avoid shadowing libvirt binding; alias the helper import.Using the name libvirt here can confuse with the official libvirt Python binding. Prefer aliasing.
-from virttest.utils_test import libvirt +from virttest.utils_test import libvirt as utils_libvirtIf you adopt this, also update usages (e.g., add_vm_device calls) accordingly in this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
🔇 Additional comments (3)
libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py (3)
43-43: Good: close session before destroying the VM.Prevents stale serial handles and avoids read/write on a torn-down domain.
79-80: Serial console re-init after restart looks good.Ensures a fresh console endpoint for the new boot.
104-107: Finally block is safe and idempotent.Guarded vm_session close and unconditional serial console cleanup are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
provider/viommu/viommu_base.py (1)
119-121: Address the previously flagged security and code quality issues.Two issues remain unresolved from the previous review:
- Line 120: The
mac_addrvariable is assigned but never used.- Line 121: Using
eval()poses a code injection risk; useast.literal_eval()instead.Please refer to the detailed remediation steps in the previous review comments.
🧹 Nitpick comments (1)
provider/viommu/viommu_base.py (1)
133-149: LGTM! Consider clarifying the in-place mutation in the docstring.The helper method correctly encapsulates the controller addressing logic. The method modifies
iface_dictin place, which is appropriate for this use case.For improved clarity, consider explicitly noting the in-place mutation in the docstring:
def _update_iface_dict_with_controller_info(self, iface_dict): """ - Update interface dictionary with controller addressing information + Update interface dictionary in place with controller addressing information :param iface_dict: Interface dictionary to update with controller info """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
provider/viommu/viommu_base.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/viommu/viommu_base.py (1)
provider/sriov/sriov_base.py (1)
parse_iface_dict(173-195)
🪛 Ruff (0.13.2)
provider/viommu/viommu_base.py
120-120: Local variable mac_addr is assigned to but never used
Remove assignment to unused variable mac_addr
(F841)
121-121: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (3)
provider/viommu/viommu_base.py (3)
111-111: LGTM!The debug log addition improves observability during VM configuration backup.
113-131: LGTM! Well-structured refactoring for multi-interface support.The refactored method correctly handles both single dict and list of dicts, maintaining backward compatibility while adding the new functionality. The extraction of controller info logic into a helper method improves maintainability.
263-263: LGTM!The debug log addition improves observability during VM configuration restoration and pairs well with the backup log in
__init__.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
provider/viommu/viommu_base.py (1)
133-149: Clarify mutation semantics and add defensive checks.The method mutates
iface_dictin-place but doesn't return a value, which can be unclear. Additionally, the guard condition on line 139 may allow unintended updates to empty dicts, and there's no validation thatcontroller_dicts[-1]contains required keys.Consider these improvements:
- Clarify docstring about in-place mutation:
def _update_iface_dict_with_controller_info(self, iface_dict): """ - Update interface dictionary with controller addressing information + Update interface dictionary in-place with controller addressing information + + Note: This method mutates the provided iface_dict and does not return a value. :param iface_dict: Interface dictionary to update with controller info """
- Add defensive checks:
- if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev': + # Only update if we have a non-hostdev interface with meaningful content + if not self.controller_dicts or not iface_dict: + return + + if iface_dict.get('type_name') == 'hostdev': + return + + last_controller = self.controller_dicts[-1] + if 'index' not in last_controller: + self.test.log.warning("Last controller missing 'index', skipping address update") + return + - iface_bus = "%0#4x" % int(self.controller_dicts[-1].get("index")) + iface_bus = "%0#4x" % int(last_controller["index"]) iface_attrs = {"bus": iface_bus} - if self.controller_dicts[-1].get("model") == "pcie-to-pci-bridge": + if last_controller.get("model") == "pcie-to-pci-bridge":
♻️ Duplicate comments (2)
provider/viommu/viommu_base.py (2)
119-120: Remove unused variablemac_addr.The
mac_addrvariable is generated but never used. If MAC address generation is needed for interface configuration, incorporate it into theiface_dictupdates; otherwise, remove this line.Apply this diff to remove the unused variable:
- # 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", "{}"))
121-121: Replaceeval()withast.literal_eval()for security.Using
eval()poses a code injection risk ifparamscan be influenced by untrusted input. Useast.literal_eval()to safely evaluate string literals.Apply this diff:
+from ast import literal_eval + def parse_iface_dict(self): """ Parse iface_dict from params, supporting both single dict and list of dicts :return: The updated iface_dict or list of iface_dicts based on type of input """ - iface_dict = eval(self.params.get("iface_dict", "{}")) + iface_dict = literal_eval(self.params.get("iface_dict", "{}"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
provider/viommu/viommu_base.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/viommu/viommu_base.py (1)
provider/sriov/sriov_base.py (1)
parse_iface_dict(173-195)
🪛 Ruff (0.13.2)
provider/viommu/viommu_base.py
120-120: Local variable mac_addr is assigned to but never used
Remove assignment to unused variable mac_addr
(F841)
121-121: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (3)
provider/viommu/viommu_base.py (3)
111-111: LGTM!The debug log appropriately documents the backup operation for troubleshooting purposes.
163-165: LGTM!The debug logs appropriately trace the controller resolution logic, making troubleshooting easier.
Also applies to: 169-169
266-266: LGTM!The debug log appropriately documents the restore operation, providing symmetric logging with the backup operation in
__init__.
| 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
provider/viommu/viommu_base.py (1)
140-146: Add defensive checks for controller attributes.The code assumes
self.controller_dicts[-1]has an "index" key and thatget_free_pci_slotsucceeds, but neither is validated. Missing keys or failures could cause unclear runtime errors.Consider adding validation:
+ last_controller = self.controller_dicts[-1] + if "index" not in last_controller: + raise KeyError(f"Controller missing 'index' key: {last_controller}") + - iface_bus = "%0#4x" % int(self.controller_dicts[-1].get("index")) + iface_bus = "%0#4x" % int(last_controller["index"]) iface_attrs = {"bus": iface_bus} - if self.controller_dicts[-1].get("model") == "pcie-to-pci-bridge": + if last_controller.get("model") == "pcie-to-pci-bridge": iface_slot = libvirt_pcicontr.get_free_pci_slot( vm_xml.VMXML.new_from_dumpxml(self.vm.name), controller_bus=iface_bus) + if not iface_slot: + raise ValueError(f"No free PCI slot available on bus {iface_bus}") iface_attrs.update({"slot": iface_slot})
♻️ Duplicate comments (3)
provider/viommu/viommu_base.py (3)
120-120: Remove unused variablemac_addr.The
mac_addrvariable is generated but never used in the method. This adds unnecessary overhead and confusion.Apply this diff to remove the unused variable:
- # 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", "{}"))
121-121: Replaceeval()withast.literal_eval()for security.Using
eval()poses a code injection risk ifparamscan be influenced by untrusted input.ast.literal_eval()safely evaluates string literals.Apply this diff:
+from ast import literal_eval + def parse_iface_dict(self): - iface_dict = eval(self.params.get("iface_dict", "{}")) + iface_dict = literal_eval(self.params.get("iface_dict", "{}"))
123-131: Add input validation and error handling.The code assumes
iface_dictis either a list or a dict but doesn't validate this assumption. If parsing yields an unexpected type, 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
provider/viommu/viommu_base.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/viommu/viommu_base.py (1)
provider/sriov/sriov_base.py (1)
parse_iface_dict(173-195)
🪛 Ruff (0.13.2)
provider/viommu/viommu_base.py
120-120: Local variable mac_addr is assigned to but never used
Remove assignment to unused variable mac_addr
(F841)
121-121: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
🔇 Additional comments (5)
provider/viommu/viommu_base.py (5)
111-111: LGTM!The debug log provides useful visibility into the backup operation.
163-165: LGTM!The added debug logs improve traceability when processing controllers with pre-controller dependencies.
186-187: LGTM!The additional debug logs provide valuable insights into controller addressing resolution.
Also applies to: 211-211
277-277: LGTM!The debug log confirms successful restoration of the VM configuration.
166-184: Verify test coverage for parent controller lookup scenarios.
No test cases were found for:
- Parent controller with
target.busNrset- Parent controller without
target.busNr(using index)- No matching parent (fallback to
get_max_contr_indexes)
Ensure tests exist for all three scenarios.
| :param iface_dict: Interface dictionary to update with controller info | ||
| """ | ||
| if self.controller_dicts and iface_dict and iface_dict.get('type_name') != 'hostdev': |
There was a problem hiding this comment.
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.
| 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'.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
provider/viommu/viommu_base.py (4)
119-120: Remove unused variablemac_addr.The
mac_addrvariable is generated but never used in the method. If MAC address generation is needed for interface configuration, it should be incorporated into theiface_dictupdates; otherwise, remove this line.Apply this diff to remove the unused variable:
- # 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", "{}"))
121-121: Replaceeval()withast.literal_eval()for security.Using
eval()to parse configuration strings poses a code injection risk ifparamscan be influenced by untrusted input.ast.literal_eval()safely evaluates string literals containing Python data structures.Apply this diff to use the safer alternative:
+from ast import literal_eval + def parse_iface_dict(self): """ Parse iface_dict from params, supporting both single dict and list of dicts :return: The updated iface_dict or list of iface_dicts based on type of input """ - iface_dict = eval(self.params.get("iface_dict", "{}")) + iface_dict = literal_eval(self.params.get("iface_dict", "{}"))
123-131: Add input validation and error handling.The code assumes
iface_dictis 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
139-139: Fix conditional check for empty dict.The condition
iface_dictwill evaluate toFalsefor 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':
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
provider/viommu/viommu_base.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/viommu/viommu_base.py (1)
provider/sriov/sriov_base.py (1)
parse_iface_dict(173-195)
🪛 Ruff (0.13.2)
provider/viommu/viommu_base.py
120-120: Local variable mac_addr is assigned to but never used
Remove assignment to unused variable mac_addr
(F841)
121-121: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (4)
provider/viommu/viommu_base.py (4)
111-111: LGTM!The debug log improves traceability during test setup.
163-165: LGTM!The additional debug logging improves traceability when debugging controller configuration issues.
Also applies to: 186-186, 211-211
288-288: LGTM!The debug log improves traceability during test teardown.
215-228: Verify busNr-based index assignment consistency
Mirrors existing parent-controller logic; no conflicting patterns found. Confirm that usingtarget.busNrasindexaligns with libvirt’s controller indexing semantics.
provider/viommu/viommu_base.py
Outdated
| parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr")) | ||
| for c in self.controller_dicts | ||
| if c["type"] == contr_dict["type"] and c["model"] == pre_controller] |
There was a problem hiding this comment.
Add safe access for controller attributes.
The list comprehension accesses c["type"] and c["model"] which could raise KeyError if these keys are missing from a controller dict. Use .get() for safer access.
Apply this diff to add safe access:
- parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr"))
- for c in self.controller_dicts
- if c["type"] == contr_dict["type"] and c["model"] == pre_controller]
+ parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr"))
+ for c in self.controller_dicts
+ if c.get("type") == contr_dict.get("type") and c.get("model") == pre_controller]📝 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.
| parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr")) | |
| for c in self.controller_dicts | |
| if c["type"] == contr_dict["type"] and c["model"] == pre_controller] | |
| parent_contrs = [(c.get("index"), c.get("target", {}).get("busNr")) | |
| for c in self.controller_dicts | |
| if c.get("type") == contr_dict.get("type") and c.get("model") == pre_controller] |
🤖 Prompt for AI Agents
In provider/viommu/viommu_base.py around lines 166 to 168, the list
comprehension uses c["type"] and c["model"] which can raise KeyError; change
those to c.get("type") and c.get("model") so missing keys return None and the
comparison is safe, preserving the existing c.get("index") and c.get("target",
{}).get("busNr") accesses; ensure the logic still filters controllers by
comparing c.get("type") == contr_dict.get("type") and c.get("model") ==
pre_controller (or contr_dict.get("model") if that was intended).
- Refactored _find_pre_controller_index into smaller, focused methods - Added flexible lookup strategies: dict format, direct index, alias name, model name, fallback - Implemented consistent error handling with warnings + fallback instead of exceptions - Added controller existence validation for explicit index references - Improved maintainability and readability of controller lookup logic - Enhanced backward compatibility with existing configurations
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
provider/viommu/viommu_base.py (3)
113-131: Previous review issues still apply; approve the list-handling approach.The issues previously flagged for lines 120-121 (unused
mac_addr, insecureeval()) and 123-131 (missing input validation) remain unaddressed. Please resolve those before merging.The overall approach to handle both single dict and list of dicts is well-designed and maintains backward compatibility.
133-149: Previous review issue still applies; approve the method extraction.The issue previously flagged for line 139 (empty dict truthiness check) remains unaddressed.
The extraction of this logic into a dedicated helper method improves code maintainability and supports the new multi-interface handling. The address computation logic is correct.
352-362: Previous review issue still applies for safe dictionary access.The issue previously flagged regarding unsafe use of
c["type"]andc["model"](which could raiseKeyError) remains unaddressed. These should use.get()for safer access.Apply this diff as suggested in previous review:
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 c.get("type") == contr_dict.get("type") and + c.get("model") == pre_controller]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
provider/viommu/viommu_base.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/viommu/viommu_base.py (1)
provider/sriov/sriov_base.py (1)
parse_iface_dict(173-195)
🪛 Ruff (0.13.3)
provider/viommu/viommu_base.py
120-120: Local variable mac_addr is assigned to but never used
Remove assignment to unused variable mac_addr
(F841)
121-121: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
🔇 Additional comments (3)
provider/viommu/viommu_base.py (3)
163-167: LGTM! Enhanced logging improves debuggability.The additional logging statements at controller processing stages will help troubleshoot pre-controller resolution and index assignment issues.
Also applies to: 192-192
269-269: LGTM! Consistent logging for teardown.The debug log confirms VM configuration restoration, maintaining consistency with setup logging.
271-362: LGTM! Well-designed flexible lookup mechanism.The multi-strategy pre-controller resolution (dict format → direct index → model name → fallback) with comprehensive logging and helper method extraction is well-architected. The separation of concerns across
_find_by_dict_format,_find_by_direct_index,_find_by_alias_name, and_find_by_model_nameimproves maintainability and testability.Once the critical and major issues are addressed, this implementation will be robust.
| # 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']}") |
There was a problem hiding this comment.
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.
| # 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.
| 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 |
There was a problem hiding this comment.
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.
| 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 _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 |
There was a problem hiding this comment.
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.
| 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_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 |
There was a problem hiding this comment.
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.
| 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.
|
backward compatibility for iommu_device_settings: (01/17) type_specific.io-github-autotest-libvirt.vIOMMU.iommu_device_settings.virtio_multi_devices.vhost_on.virtio: STARTED |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes