Conversation
|
libvirt-11.10.0 on aarch64 Part test results: (1/2) type_specific.io-github-autotest-libvirt.sriov.vm_lifecycle.reboot.network_interface.network.vf_address.hostdev.managed_yes.without_iommu.with_iommufd: PASS (96.29 s) |
WalkthroughAdds two top-level variants to the SR-IOV reboot test config: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
provider/sriov/sriov_base.py (1)
195-195: Preferast.literal_evaloverevalfor dict literals from params.Ruff S307 flags this. While the pattern is consistent with lines 186 and 194 in the same method, replacing all three occurrences with
ast.literal_evalis safer and sufficient for dict/string literals.♻️ Proposed refactor (apply to lines 186, 194, and 195)
+import ast ... - iface_dict = eval(self.params.get('iface_dict', '{}')) + iface_dict = ast.literal_eval(self.params.get('iface_dict', '{}')) ... - iface_dict = eval(self.params.get('hostdev_dict', '{}')) + iface_dict = ast.literal_eval(self.params.get('hostdev_dict', '{}')) ... - driver_dict = eval(self.params.get("driver_dict", "{}")) + driver_dict = ast.literal_eval(self.params.get("driver_dict", "{}"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` at line 195, Replace unsafe eval calls that parse dict/string literals from params with ast.literal_eval: specifically change driver_dict = eval(self.params.get("driver_dict", \"{}\")) and the two other eval(...) usages in the same method to use ast.literal_eval(self.params.get(...)). Add import ast at the top of the module if it's not already present, and ensure behavior is identical for dict/string literals while avoiding arbitrary code execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/sriov/sriov_base.py`:
- Around line 195-197: The current shallow merge in parse_iface_dict uses
driver_dict = eval(self.params.get("driver_dict", "{}")) followed by
iface_dict.update(driver_dict), which replaces nested keys (e.g., 'driver') and
drops existing 'driver_attr' entries; change this to perform a deep/recursive
merge of driver_dict into iface_dict so nested dicts (like
iface_dict['driver']['driver_attr']) are merged rather than overwritten —
implement or call a utility deep_merge/deep_update function and use it instead
of dict.update to merge driver_dict into iface_dict, ensuring symbols referenced
are driver_dict, iface_dict, and the parse_iface_dict method.
---
Nitpick comments:
In `@provider/sriov/sriov_base.py`:
- Line 195: Replace unsafe eval calls that parse dict/string literals from
params with ast.literal_eval: specifically change driver_dict =
eval(self.params.get("driver_dict", \"{}\")) and the two other eval(...) usages
in the same method to use ast.literal_eval(self.params.get(...)). Add import ast
at the top of the module if it's not already present, and ensure behavior is
identical for dict/string literals while avoiding arbitrary code execution.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.cfglibvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.pyprovider/sriov/sriov_base.py
dzhengfy
force-pushed
the
enable_iommufd
branch
from
7070dcd to
3951000
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
provider/sriov/sriov_base.py (1)
195-197: Shallowdict.update()silently drops existingdriver_attrvalues — see prior review comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` around lines 195 - 197, The current shallow merge using iface_dict.update(driver_dict) overwrites existing driver_attr values; change this to a merge that preserves existing iface_dict entries (especially 'driver_attr') by only setting keys from driver_dict when they are absent or when the value is non-conflicting; implement a small merge helper or inline logic that iterates driver_dict items and assigns iface_dict[k] = v only if k not in iface_dict (or handle nested/driver_attr merging explicitly) and replace the eval(self.params.get("driver_dict", "{}")) usage with the resulting driver_dict variable referenced in this file so the new logic runs where driver_dict and iface_dict are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/cfg/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.cfg`:
- Line 8: The version guard func_supported_since_libvirt_ver is set incorrectly
to (11, 10, 0); update it to (12, 1, 0) so the test is skipped on libvirt
releases older than the iommufd introduction — change the tuple value used by
func_supported_since_libvirt_ver in sriov_vm_lifecycle_reboot.cfg to (12, 1, 0).
In `@provider/sriov/sriov_base.py`:
- Line 195: Replace the unsafe eval() call used to parse driver_dict with
ast.literal_eval: import ast at the top of the module and change driver_dict =
eval(self.params.get("driver_dict", "{}")) to use
ast.literal_eval(self.params.get("driver_dict", "{}")). Wrap the literal_eval
call in a try/except that catches (ValueError, SyntaxError, TypeError) and falls
back to an empty dict (and optionally logs the parsing error) so driver_dict
remains a safe dict when parsing fails; reference the driver_dict variable and
the self.params.get(...) usage in sriov_base.py for the location to change.
---
Duplicate comments:
In `@provider/sriov/sriov_base.py`:
- Around line 195-197: The current shallow merge using
iface_dict.update(driver_dict) overwrites existing driver_attr values; change
this to a merge that preserves existing iface_dict entries (especially
'driver_attr') by only setting keys from driver_dict when they are absent or
when the value is non-conflicting; implement a small merge helper or inline
logic that iterates driver_dict items and assigns iface_dict[k] = v only if k
not in iface_dict (or handle nested/driver_attr merging explicitly) and replace
the eval(self.params.get("driver_dict", "{}")) usage with the resulting
driver_dict variable referenced in this file so the new logic runs where
driver_dict and iface_dict are used.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.cfglibvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.pyprovider/sriov/sriov_base.py
| only x86_64, aarch64 | ||
| variants: | ||
| - with_iommufd: | ||
| func_supported_since_libvirt_ver = (11, 10, 0) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
libvirt 12.1.0 release iommufd hostdev driver attribute
💡 Result:
In libvirt 12.1.0, IOMMUFD support for VFIO PCI hostdev is enabled via an <iommufd/> sub-element inside the hostdev <driver> element (QEMU/KVM only) — it is not an attribute in 12.1.0. When present, libvirt opens /dev/iommu and the VFIO device cdev and passes the FDs to QEMU. [1]
Example (12.1.0 domain XML):
<hostdev mode='subsystem' type='pci' managed='yes'>
<driver model='vfio-pci'>
<iommufd/>
</driver>
<source>
<address domain='0x0000' bus='0x03' slot='0x00' function='0x0'/>
</source>
</hostdev>[1]
Note: there has been later development discussion/patches proposing an iommufd='yes' attribute on <driver/>, but that’s separate from what 12.1.0 documents/ships. [2]
Sources: [1] [2]
🌐 Web query:
libvirt iommufd attribute driver_attr "iommufd='yes'" release version
💡 Result:
Libvirt’s Domain XML support for enabling the IOMMUFD backend for VFIO hostdev devices (the feature behind using iommufd such as <driver iommufd='yes'/>) is documented as :since: 12.1.0 (QEMU/KVM only). (gitlab.com)
As of today, libvirt 12.1.0 is still “unreleased”, while 12.0.0 was released on 2026-01-15, so this is not in any released libvirt version yet (it’s on the development branch heading toward 12.1.0). (libvirt.org)
The corresponding implementation was discussed and posted as a patch series on the libvirt-devel mailing list in late 2025. (mail-archive.com)
Citations:
- 1: https://gitlab.com/libvirt/libvirt/-/raw/master/docs/formatdomain.rst
- 2: https://www.libvirt.org/news.html
- 3: https://www.mail-archive.com/devel%40lists.libvirt.org/msg13887.html?utm_source=openai
Version guard (11, 10, 0) is incorrect — libvirt documentation marks iommufd as introduced in 12.1.0.
The upstream libvirt formatdomain.rst documentation explicitly marks the iommufd attribute for hostdev driver as :since: 12.1.0 (QEMU and KVM only). Setting the guard at (11, 10, 0) will cause the test to run on libvirt versions that do not support this feature, resulting in schema validation or runtime errors instead of a clean skip.
Note: libvirt 12.1.0 is currently unreleased (in development as of February 2026), so this test targets a feature not yet in a stable release.
🐛 Proposed fix
- func_supported_since_libvirt_ver = (11, 10, 0)
+ func_supported_since_libvirt_ver = (12, 1, 0)📝 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.
| func_supported_since_libvirt_ver = (11, 10, 0) | |
| func_supported_since_libvirt_ver = (12, 1, 0) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libvirt/tests/cfg/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.cfg` at line
8, The version guard func_supported_since_libvirt_ver is set incorrectly to (11,
10, 0); update it to (12, 1, 0) so the test is skipped on libvirt releases older
than the iommufd introduction — change the tuple value used by
func_supported_since_libvirt_ver in sriov_vm_lifecycle_reboot.cfg to (12, 1, 0).
| if vf_pci_addr2.get('type'): | ||
| del vf_pci_addr2['type'] | ||
| iface_dict = eval(self.params.get('hostdev_dict', '{}')) | ||
| driver_dict = eval(self.params.get("driver_dict", "{}")) |
There was a problem hiding this comment.
Replace eval() with ast.literal_eval() to avoid arbitrary code execution.
The parameter value for driver_dict is sourced from the test params dict. Using eval() on it allows execution of arbitrary Python expressions. ast.literal_eval is the safe drop-in that only parses literals.
🛡️ Proposed fix
+import ast
- driver_dict = eval(self.params.get("driver_dict", "{}"))
+ driver_dict = ast.literal_eval(self.params.get("driver_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.
| driver_dict = eval(self.params.get("driver_dict", "{}")) | |
| import ast | |
| driver_dict = ast.literal_eval(self.params.get("driver_dict", "{}")) |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 195-195: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@provider/sriov/sriov_base.py` at line 195, Replace the unsafe eval() call
used to parse driver_dict with ast.literal_eval: import ast at the top of the
module and change driver_dict = eval(self.params.get("driver_dict", "{}")) to
use ast.literal_eval(self.params.get("driver_dict", "{}")). Wrap the
literal_eval call in a try/except that catches (ValueError, SyntaxError,
TypeError) and falls back to an empty dict (and optionally logs the parsing
error) so driver_dict remains a safe dict when parsing fails; reference the
driver_dict variable and the self.params.get(...) usage in sriov_base.py for the
location to change.
|
I will provide more test results after CI run. |
dzhengfy
force-pushed
the
enable_iommufd
branch
from
3951000 to
253b4bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/sriov/sriov_base.py`:
- Around line 205-208: The code accesses nested keys without checks—when merging
driver attributes in sriov_base.py (variables existing_driver_dict,
existing_attrs_dict, iface_dict, driver_dict) add defensive null checks: ensure
iface_dict.get("driver") and driver_dict.get("driver") return dicts and that
their "driver_attr" entries exist (create an empty dict if missing) before
iterating; then safely update the existing_attrs_dict from
driver_dict["driver"]["driver_attr"] only when it is a dict. Also handle cases
where existing_driver_dict is None by initializing it to a dict with a
"driver_attr" dict so existing_attrs_dict.update(...) cannot raise
AttributeError.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.cfglibvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.pyprovider/sriov/sriov_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.py
provider/sriov/sriov_base.py
Outdated
| existing_driver_dict = iface_dict.get("driver") | ||
| existing_attrs_dict = existing_driver_dict.get("driver_attr") | ||
| for attr_key, attr_value in driver_dict.get("driver").get("driver_attr").items(): | ||
| existing_attrs_dict.update({attr_key:attr_value}) |
There was a problem hiding this comment.
Missing null checks may cause AttributeError if nested keys are absent.
The code assumes both iface_dict['driver']['driver_attr'] and driver_dict['driver']['driver_attr'] always exist. If either dict has {'driver': {}} without driver_attr, this will crash.
🛡️ Proposed defensive fix
if iface_dict.get("driver"):
# We need to merge the key/values for driver_attr.
- # For example:
- # iface_dict = {'driver': {'driver_attr': {'name': 'vfio'}}}
- # driver_dict = {'driver': {'driver_attr': {'iommufd': 'yes'}}}
- # The expected result:
- # iface_dict = {'driver': {'driver_attr': {'name': 'vfio', 'iommufd': 'yes'}}}
existing_driver_dict = iface_dict.get("driver")
- existing_attrs_dict = existing_driver_dict.get("driver_attr")
- for attr_key, attr_value in driver_dict.get("driver").get("driver_attr").items():
- existing_attrs_dict.update({attr_key:attr_value})
+ existing_attrs_dict = existing_driver_dict.get("driver_attr", {})
+ new_driver = driver_dict.get("driver", {})
+ new_attrs = new_driver.get("driver_attr", {})
+ existing_attrs_dict.update(new_attrs)
+ existing_driver_dict["driver_attr"] = existing_attrs_dict
else:
iface_dict.update(driver_dict)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@provider/sriov/sriov_base.py` around lines 205 - 208, The code accesses
nested keys without checks—when merging driver attributes in sriov_base.py
(variables existing_driver_dict, existing_attrs_dict, iface_dict, driver_dict)
add defensive null checks: ensure iface_dict.get("driver") and
driver_dict.get("driver") return dicts and that their "driver_attr" entries
exist (create an empty dict if missing) before iterating; then safely update the
existing_attrs_dict from driver_dict["driver"]["driver_attr"] only when it is a
dict. Also handle cases where existing_driver_dict is None by initializing it to
a dict with a "driver_attr" dict so existing_attrs_dict.update(...) cannot raise
AttributeError.
The IOMMUFD is a new machinism and implementation for VFIO passthrough/hotplug. We enable some existing test cases with iommufd setting. Signed-off-by: Dan Zheng <dzheng@redhat.com>
dzhengfy
force-pushed
the
enable_iommufd
branch
from
253b4bf to
d00f8bf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
provider/sriov/sriov_base.py (2)
196-196: 🛠️ Refactor suggestion | 🟠 MajorReplace
eval()withast.literal_eval()to avoid arbitrary code execution.Static analysis flags this as insecure.
ast.literal_evalsafely parses literal structures and is the appropriate choice here.🛡️ Proposed fix
+import ast + # At line 196: - driver_dict = eval(self.params.get("driver_dict", "{}")) + driver_dict = ast.literal_eval(self.params.get("driver_dict", "{}"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` at line 196, Replace the unsafe eval call when parsing driver_dict with ast.literal_eval: instead of driver_dict = eval(self.params.get("driver_dict", "{}")), import ast and use driver_dict = ast.literal_eval(self.params.get("driver_dict", "{}")) and wrap it in a small try/except (catching ValueError and SyntaxError) to fall back to {} or log a parsing error; refer to the driver_dict variable and the self.params.get(...) call to locate the change.
205-208:⚠️ Potential issue | 🟡 MinorMissing null checks may cause
AttributeErrorif nested keys are absent.Line 207 chains
.get("driver").get("driver_attr").items()without null checks. Ifdriver_dictis{'driver': {}}(nodriver_attrkey), calling.items()onNonewill crash. Similarly, line 206 could yieldNoneforexisting_attrs_dict, causing line 208's.update()to fail.🛡️ Proposed defensive fix
if iface_dict.get("driver"): existing_driver_dict = iface_dict.get("driver") - existing_attrs_dict = existing_driver_dict.get("driver_attr") - for attr_key, attr_value in driver_dict.get("driver").get("driver_attr").items(): - existing_attrs_dict.update({attr_key: attr_value}) + existing_attrs_dict = existing_driver_dict.setdefault("driver_attr", {}) + new_driver = driver_dict.get("driver", {}) + new_attrs = new_driver.get("driver_attr", {}) + existing_attrs_dict.update(new_attrs) else: iface_dict.update(driver_dict)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/sriov/sriov_base.py` around lines 205 - 208, The code assumes nested keys exist when merging driver attributes: replace the unsafe chains around iface_dict, existing_driver_dict and driver_dict (specifically the use of existing_driver_dict.get("driver_attr") and driver_dict.get("driver").get("driver_attr").items()) with defensive checks or safe defaults; for example, ensure existing_driver_dict is present, obtain existing_attrs_dict = existing_driver_dict.setdefault("driver_attr", {}) (or fall back to {}), and iterate over driver_dict.get("driver", {}).get("driver_attr", {}).items() only when that returns a dict, so update existing_attrs_dict only when not None to avoid AttributeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@provider/sriov/sriov_base.py`:
- Line 196: Replace the unsafe eval call when parsing driver_dict with
ast.literal_eval: instead of driver_dict = eval(self.params.get("driver_dict",
"{}")), import ast and use driver_dict =
ast.literal_eval(self.params.get("driver_dict", "{}")) and wrap it in a small
try/except (catching ValueError and SyntaxError) to fall back to {} or log a
parsing error; refer to the driver_dict variable and the self.params.get(...)
call to locate the change.
- Around line 205-208: The code assumes nested keys exist when merging driver
attributes: replace the unsafe chains around iface_dict, existing_driver_dict
and driver_dict (specifically the use of existing_driver_dict.get("driver_attr")
and driver_dict.get("driver").get("driver_attr").items()) with defensive checks
or safe defaults; for example, ensure existing_driver_dict is present, obtain
existing_attrs_dict = existing_driver_dict.setdefault("driver_attr", {}) (or
fall back to {}), and iterate over driver_dict.get("driver",
{}).get("driver_attr", {}).items() only when that returns a dict, so update
existing_attrs_dict only when not None to avoid AttributeError.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.cfglibvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.pyprovider/sriov/sriov_base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.py
1 participant
The IOMMUFD is a new machinism and implementation for VFIO passthrough/hotplug. We enable some existing test cases with iommufd setting.
We need to make sure both traditional and IOMMUFD can work.
Signed-off-by: Dan Zheng dzheng@redhat.com
Summary by CodeRabbit
New Features
Tests