Migrate netperf perf tests from tp-qemu to tp-libvirt #6743
Conversation
WalkthroughAdds a netperf test suite and multiple provider utilities for libvirt QEMU virtual networking. New test config: libvirt/tests/cfg/virtual_network/qemu/netperf.cfg. New test runner: libvirt/tests/src/virtual_network/qemu/netperf.py (exports run, start_test, launch_client) implementing VM boot/pinning, environment prep, netserver/client lifecycle, result collection, and VMXML/NUMA handling. New providers: provider/virtual_network/netperf_base.py (env setup, ssh helpers, result formatting), provider/virtual_network/network_base.py (vCPU and vhost thread pinning), provider/virtual_network/vdpa_utils.py (OVS/vdpa helpers), and provider/windows/win_driver_utils.py (Windows VirtIO driver management). Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 7
🧹 Nitpick comments (9)
provider/virtual_network/network_base.py (2)
635-641: Consider addingstrict=Truetozip()for safety.If
vhost_tidsandcpushave mismatched lengths, the current code silently ignores extra items. Addingstrict=True(Python 3.10+) would catch unexpected mismatches, though the caller already guards against this.🔎 Proposed fix (if Python 3.10+ is required)
- for tid, cpu in zip(vhost_tids, cpus): + for tid, cpu in zip(vhost_tids, cpus, strict=True):
673-674: Useraise ... from efor proper exception chaining.Catching bare
Exceptionand re-raising withoutfrom eloses the original traceback context, making debugging harder.🔎 Proposed fix
except Exception as e: - raise exceptions.TestError("Failed to pin VM threads: %s" % e) + raise exceptions.TestError("Failed to pin VM threads: %s" % e) from eprovider/windows/win_driver_utils.py (1)
448-457: Consider session lifecycle management.The function opens a session at line 448 but does not close it in the success path (only
session.close()is called at line 458). Ifload_driverthrows an exception, the session fromunload_driver(line 449) could be leaked.provider/virtual_network/netperf_base.py (1)
103-119: Consider usingnumbers.Numberfor numeric type checking.The current implementation checks
intandfloatseparately. Usingisinstance(result, numbers.Number)after checking for strings would handle all numeric types includingDecimal.import numbers # ... elif isinstance(result, numbers.Number): if isinstance(result, int): value = "%" + base + "d" else: value = "%" + base + "." + fbase + "f"libvirt/tests/src/virtual_network/qemu/netperf.py (5)
293-310: Unused loop variable and potential cleanup.The loop variable
iat line 295 is not used within the loop body. Use_to indicate an intentional unused variable.🔎 Proposed fix
- for i in range(2): + for _ in range(2): if client != "localhost" and params.get("os_type_client") == "linux":
555-566: Ambiguous parameter name and unused arguments.
- Parameter
l(line 561) is ambiguous per PEP8 (looks like1). Consider renaming totest_duration.- Parameter
port(line 563) is declared but never used in the function.🔎 Proposed fix
def launch_client( sessions, server, server_ctl, host, clients, - l, + test_duration, nf_args, - port, params, test, ):Update the caller at line 489-500 accordingly and replace all uses of
lwithtest_durationwithin the function.
580-580: Remove unused variable assignment.The
timeoutvariable is assigned but never used.🔎 Proposed fix
if params.get("os_type") == "windows": - timeout = float(params.get("timeout", "240")) cdrom_drv = utils_misc.get_winutils_vol(server_ctl)
371-373: Use context manager for file handling.The file opened at line 371 should use a context manager to ensure proper cleanup on exceptions.
🔎 Proposed fix
- logfile = open(path, "a+") - logfile.write(output) - logfile.close() + with open(path, "a+") as logfile: + logfile.write(output)
722-724: Use context manager for file handling inparse_demo_result.Similar to above, use a context manager for safer file handling.
🔎 Proposed fix
- fd = open(fname) - lines = fd.readlines() - fd.close() + with open(fname) as fd: + lines = fd.readlines()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.pyprovider/virtual_network/vdpa_utils.pyprovider/windows/win_driver_utils.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
🧬 Code graph analysis (2)
provider/windows/win_driver_utils.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
libvirt/tests/src/virtual_network/qemu/netperf.py (8)
provider/virtual_network/netperf_base.py (3)
ssh_cmd(71-85)netperf_record(122-147)netperf_thread(88-100)provider/virtual_network/vdpa_utils.py (2)
OVSHandler(84-132)get_vdpa_ovs_info(92-132)provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)provider/windows/win_driver_utils.py (2)
get_driver_inf_path(123-155)uninstall_driver(77-120)provider/virtual_network/network_base.py (1)
pin_vcpu_vhost_threads(644-674)libvirt/tests/src/passthrough/robustness/passthrough_robustness.py (1)
start(176-225)libvirt/tests/src/virsh_cmd/network/virsh_net_dhcp_leases.py (1)
f(149-150)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/netperf.py
99-99: Function call with shell=True parameter identified, security issue
(S604)
174-174: Loop control variable driver_name overrides iterable it iterates
(B020)
264-264: Function call with shell=True parameter identified, security issue
(S604)
269-269: Function call with shell=True parameter identified, security issue
(S604)
295-295: Loop control variable i not used within loop body
(B007)
360-360: Function call with shell=True parameter identified, security issue
(S604)
367-367: Probable insecure usage of temporary file or directory: "/tmp/log_guestinfo.sh"
(S108)
537-537: Function call with shell=True parameter identified, security issue
(S604)
561-561: Ambiguous variable name: l
(E741)
563-563: Unused function argument: port
(ARG001)
570-570: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netperf"
(S108)
571-571: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netserver"
(S108)
580-580: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
681-681: Unused function argument: timeout
(ARG001)
682-682: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
684-684: Probable insecure usage of temporary file or directory: "/tmp/netperf_agent.py %d %s -D 1 -H %s -l %s %s"
(S108)
697-697: Do not use bare except
(E722)
749-749: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
provider/virtual_network/netperf_base.py
43-43: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
47-47: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
80-80: Function call with shell=True parameter identified, security issue
(S604)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
provider/virtual_network/network_base.py
621-621: Function call with shell=True parameter identified, security issue
(S604)
635-635: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
637-637: Function call with shell=True parameter identified, security issue
(S604)
673-673: Do not catch blind exception: Exception
(BLE001)
674-674: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
provider/virtual_network/vdpa_utils.py
16-16: Function call with shell=True parameter identified, security issue
(S604)
33-33: Function call with shell=True parameter identified, security issue
(S604)
49-49: Function call with shell=True parameter identified, security issue
(S604)
63-63: Function call with shell=True parameter identified, security issue
(S604)
66-66: Unused function argument: ovs
(ARG001)
🔇 Additional comments (6)
provider/virtual_network/network_base.py (1)
616-624: Approve: Robust vhost thread detection with fallback.The implementation correctly handles both old (qemu user threads) and new (kernel threads) vhost patterns with a fallback mechanism. The use of
ignore_status=Trueand safe parsing ensures graceful handling when no vhost threads exist.provider/virtual_network/vdpa_utils.py (1)
84-131: LGTM: Clean OVS handler implementation.The
OVSHandlerclass properly encapsulates OVS operations with graceful fallback when OVS is unavailable. Theget_vdpa_ovs_infomethod handles both vdpa and non-vdpa network types with appropriate error handling forNotImplementedError.provider/windows/win_driver_utils.py (1)
24-63: LGTM: Comprehensive VirtIO driver mapping.The
driver_info_dictprovides a well-structured mapping of VirtIO device names to their hardware IDs and friendly names, covering the common VirtIO device types.libvirt/tests/cfg/virtual_network/qemu/netperf.cfg (1)
1-116: LGTM: Well-structured netperf test configuration.The configuration properly defines test parameters including protocol types, session counts, message sizes, and platform-specific settings. The use of Cartesian variants for jumbo frame testing and Windows/Linux differentiation follows tp-libvirt conventions.
provider/virtual_network/netperf_base.py (1)
71-85: LGTM: Clean SSH command abstraction.The
ssh_cmdfunction properly abstracts local vs remote execution, handling bothlocalhoststring and session objects. Theignore_statusparameter is correctly mapped to the appropriate underlying parameter for each execution path.libvirt/tests/src/virtual_network/qemu/netperf.py (1)
40-53: LGTM: Well-structured test entry point.The
runfunction provides clear documentation of the test flow and properly uses error context decorators for traceability.
| # log_guestinfo_script = scripts/rh_perf_log_guestinfo_script.bat | ||
| # log_guestinfo_exec = cmd /c | ||
| # log_guestinfo_path = C:\log_guestinfo.bat | ||
| server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s" |
There was a problem hiding this comment.
Windows MTU command has mismatched quotes.
The command string has an opening " before %s but no matching close before the second %s, which will cause the command to fail on Windows.
🔎 Proposed fix
- server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s"
+ server_mtu_cmd = "netsh interface ipv4 set interface \"%s\" mtu=%s"📝 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.
| server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s" | |
| server_mtu_cmd = "netsh interface ipv4 set interface \"%s\" mtu=%s" |
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/netperf.cfg around line 99, the
server_mtu_cmd string has mismatched quotes around the interface placeholder
("%s"); fix by ensuring the inner quotes are escaped or by using single quotes
for the outer string so the command becomes e.g. "netsh interface ipv4 set
interface \"%s\" mtu=%s" or 'netsh interface ipv4 set interface "%s" mtu=%s' so
the interface name is properly quoted.
| ret["thu"] = parse_demo_result(fname, int(sessions)) | ||
| return ret | ||
| break |
There was a problem hiding this comment.
Unreachable code after return.
The break statement at line 810 is unreachable because it follows a return statement. Remove the dead code.
🔎 Proposed fix
ret["thu"] = parse_demo_result(fname, int(sessions))
return ret
- break
else:📝 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.
| ret["thu"] = parse_demo_result(fname, int(sessions)) | |
| return ret | |
| break | |
| ret["thu"] = parse_demo_result(fname, int(sessions)) | |
| return ret | |
| else: |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/netperf.py around lines 808 to 810,
there's an unreachable break statement immediately after a return; remove the
dead break to clean up the control flow and ensure no unreachable code remains.
| def add_flows_to_ovs_bridge(bridge, ovs): | ||
| """ | ||
| Add flow rules to the given ovs bridge. | ||
| :parma bridge: OVS bridge name | ||
| :type bridge: str | ||
| :param ovs: OVS instance | ||
| :type ovs: OpenVSwitch | ||
| """ | ||
| utils_net.openflow_manager( | ||
| bridge, "add-flow", flow_options="in_port=1,idle_timeout=0,actions=output:2" | ||
| ) | ||
| utils_net.openflow_manager( | ||
| bridge, "add-flow", flow_options="in_port=2,idle_timeout=0,actions=output:1" | ||
| ) | ||
| utils_net.openflow_manager(bridge, "dump-flows") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused ovs parameter.
The ovs parameter is declared but never used in the function body. The flow operations use utils_net.openflow_manager directly without needing the OVS instance.
🔎 Proposed fix
-def add_flows_to_ovs_bridge(bridge, ovs):
+def add_flows_to_ovs_bridge(bridge):
"""
Add flow rules to the given ovs bridge.
- :parma bridge: OVS bridge name
+ :param bridge: OVS bridge name
:type bridge: str
- :param ovs: OVS instance
- :type ovs: OpenVSwitch
"""Also update the caller at line 125:
- add_flows_to_ovs_bridge(ovs_br, self.ovs)
+ add_flows_to_ovs_bridge(ovs_br)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
66-66: Unused function argument: ovs
(ARG001)
🤖 Prompt for AI Agents
In provider/virtual_network/vdpa_utils.py around lines 66 to 81, the function
add_flows_to_ovs_bridge declares an unused ovs parameter; remove the ovs
parameter from the function signature and its docstring, update the function to
only accept bridge (and maintain the same body), and update all callers (notably
the call at line 125) to stop passing the ovs argument so they call
add_flows_to_ovs_bridge(bridge) instead; ensure imports and tests still pass
after this change.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
provider/windows/win_driver_utils.py (1)
400-404: Bug: Discarded return value and missing function arguments.This issue was previously identified. Line 401 discards the
params.get()result, and line 404 callsinstall_driver_by_virtio_mediawith 4 arguments when the function signature requires 6 (session, test, devcon_path, media_type, driver_name, device_hwid).provider/virtual_network/network_base.py (1)
593-596: Incorrect variable logged inpin_vcpus.The log message uses
vcpu_countinstead ofvcpu, logging the same total for every iteration rather than the current vCPU index.provider/virtual_network/vdpa_utils.py (1)
66-81: Remove unusedovsparameter.The
ovsparameter is declared but never used in the function body.libvirt/tests/src/virtual_network/qemu/netperf.py (3)
172-174: Bug: Loop variable shadows outerdriver_namevariable.The loop reuses
driver_nameas the loop variable, overwriting the original parameter value.
694-702: Bareexceptclause hides errors.The bare
exceptcatches all exceptions includingKeyboardInterruptandSystemExit, which can mask real issues.
805-810: Unreachable code afterreturnand file handling issue.The
breakat line 810 is unreachable. Also, use a context manager for the file at lines 805-807.
🧹 Nitpick comments (8)
provider/windows/win_driver_utils.py (2)
192-192: Simplify boolean assignment.
installed_any |= Trueis functionally correct but unusual. Sincex | Trueis alwaysTrue, this can be simplified.🔎 Proposed fix
- installed_any |= True + installed_any = True
451-454: Use Pythonic boolean check.
is Falsecomparison works but is not idiomatic Python. PEP 8 recommends usingif not x:for boolean checks.🔎 Proposed fix
- if vm.is_alive() is False: + if not vm.is_alive(): test.fail( "VM is not alive after uninstall driver,please check if it is a memory leak" )provider/virtual_network/network_base.py (2)
635-641: Consider addingstrict=Truetozip()for safety.While the current caller ensures equal-length lists, adding
strict=Truewould catch mismatches from future callers and make the contract explicit.🔎 Proposed fix
- for tid, cpu in zip(vhost_tids, cpus): + for tid, cpu in zip(vhost_tids, cpus, strict=True):
673-674: Chain the exception to preserve the original traceback.The current code loses the original exception context, making debugging harder. Use
raise ... from eto preserve the chain.🔎 Proposed fix
- except Exception as e: - raise exceptions.TestError("Failed to pin VM threads: %s" % e) + except Exception as e: + raise exceptions.TestError("Failed to pin VM threads: %s" % e) from elibvirt/tests/src/virtual_network/qemu/netperf.py (4)
371-373: Use a context manager for file handling.Opening files without a context manager risks leaving the file handle open if an exception occurs.
🔎 Proposed fix
- logfile = open(path, "a+") - logfile.write(output) - logfile.close() + with open(path, "a+") as logfile: + logfile.write(output)
427-427: Use a context manager for the result file.The file is opened at line 427 and closed at line 551. If an exception occurs in between, the file handle leaks. Use a context manager or wrap in try/finally.
561-566: Remove or use theportparameter.The
portparameter (line 563) is declared but never used. It appears intended fornetserver_port, which is passed separately. Either removeportor use it in the netserver start logic.🔎 Proposed fix
def launch_client( sessions, server, server_ctl, host, clients, l, nf_args, - port, + netserver_port, params, test, ):Then use
netserver_portwhere needed, or simply remove if truly unused.
722-724: Use a context manager for file handling.Opening files without a context manager risks leaving the file handle open if an exception occurs.
🔎 Proposed fix
- fd = open(fname) - lines = fd.readlines() - fd.close() + with open(fname) as fd: + lines = fd.readlines()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.pyprovider/virtual_network/vdpa_utils.pyprovider/windows/win_driver_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
🪛 Ruff (0.14.10)
provider/virtual_network/netperf_base.py
43-43: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
47-47: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
80-80: Function call with shell=True parameter identified, security issue
(S604)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
provider/virtual_network/network_base.py
621-621: Function call with shell=True parameter identified, security issue
(S604)
635-635: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
637-637: Function call with shell=True parameter identified, security issue
(S604)
673-673: Do not catch blind exception: Exception
(BLE001)
674-674: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
libvirt/tests/src/virtual_network/qemu/netperf.py
99-99: Function call with shell=True parameter identified, security issue
(S604)
174-174: Loop control variable driver_name overrides iterable it iterates
(B020)
264-264: Function call with shell=True parameter identified, security issue
(S604)
269-269: Function call with shell=True parameter identified, security issue
(S604)
295-295: Loop control variable i not used within loop body
(B007)
360-360: Function call with shell=True parameter identified, security issue
(S604)
367-367: Probable insecure usage of temporary file or directory: "/tmp/log_guestinfo.sh"
(S108)
537-537: Function call with shell=True parameter identified, security issue
(S604)
561-561: Ambiguous variable name: l
(E741)
563-563: Unused function argument: port
(ARG001)
570-570: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netperf"
(S108)
571-571: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netserver"
(S108)
580-580: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
681-681: Unused function argument: timeout
(ARG001)
682-682: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
684-684: Probable insecure usage of temporary file or directory: "/tmp/netperf_agent.py %d %s -D 1 -H %s -l %s %s"
(S108)
697-697: Do not use bare except
(E722)
749-749: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
provider/virtual_network/vdpa_utils.py
16-16: Function call with shell=True parameter identified, security issue
(S604)
33-33: Function call with shell=True parameter identified, security issue
(S604)
49-49: Function call with shell=True parameter identified, security issue
(S604)
63-63: Function call with shell=True parameter identified, security issue
(S604)
66-66: Unused function argument: ovs
(ARG001)
🔇 Additional comments (5)
provider/virtual_network/network_base.py (1)
600-624: LGTM!The dual-pattern approach for detecting vhost threads (both legacy QEMU user threads and newer kernel threads) is well-designed and handles the empty case gracefully.
provider/virtual_network/vdpa_utils.py (1)
9-64: LGTM!The helper functions appropriately query system state for OVS and PCI device information. The shell commands use trusted inputs from the test framework.
provider/virtual_network/netperf_base.py (3)
71-85: LGTM!The
ssh_cmdfunction correctly abstracts command execution for both local and remote contexts, with proper timeout and error handling support.
103-119: LGTM!The
format_resultfunction correctly handles different types with appropriate format specifiers and provides clear error messaging for unsupported types.
4-4: Verify ifsixdependency can be removed based on Python version requirements.The
sixlibrary is used for Python 2/3 compatibility. If the project only supports Python 3, replacesix.string_typeswithstr. Checksetup.py,setup.cfg, orpyproject.tomlforpython_requiresto confirm minimum Python version before removing this dependency.
| global _netserver_started | ||
| # Start netserver | ||
| if _netserver_started: | ||
| test.log.debug("Netserver already started.") | ||
| else: | ||
| error_context.context("Start Netserver on guest", test.log.info) | ||
| if params.get("os_type") == "windows": | ||
| timeout = float(params.get("timeout", "240")) | ||
| cdrom_drv = utils_misc.get_winutils_vol(server_ctl) | ||
| start_session = server_ctl | ||
| netserv_start_cmd = params.get("netserv_start_cmd") % cdrom_drv | ||
| test.log.info( | ||
| "Start netserver on windows guest, cmd is: %s", netserv_start_cmd | ||
| ) | ||
| start_netserver_win(start_session, netserv_start_cmd, test) | ||
| else: | ||
| test.log.info("Netserver start cmd is '%s'", server_path) | ||
| netperf_base.ssh_cmd(server_ctl, "pidof netserver || %s" % server_path) | ||
| ncpu = netperf_base.ssh_cmd( | ||
| server_ctl, "cat /proc/cpuinfo |grep processor |wc -l" | ||
| ) | ||
| ncpu = re.findall(r"\d+", ncpu)[-1] | ||
|
|
||
| test.log.info("Netserver start successfully") |
There was a problem hiding this comment.
Global flag _netserver_started is checked but never set to True.
The code checks _netserver_started at line 575 but never sets it to True after successfully starting the netserver. This means the netserver start logic runs on every call to launch_client, which may be wasteful or cause issues if the server is already running.
🔎 Proposed fix
test.log.info("Netserver start successfully")
+ _netserver_started = TrueAdd global _netserver_started at the beginning of launch_client if modifying the global:
def launch_client(...):
"""Launch netperf clients"""
+ global _netserver_started
netperf_version = params.get("netperf_version", "2.6.0")🧰 Tools
🪛 Ruff (0.14.10)
580-580: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/netperf.py around lines 573 to 596,
the global flag _netserver_started is checked but never set true after starting
the netserver; after successfully starting netserver (both the Windows branch
after start_netserver_win and the non-Windows branch after invoking server_path
and verifying start), set _netserver_started = True (declare global already
present) so subsequent calls skip restart; place the assignment just after the
"Netserver start successfully" log (or immediately after each successful start
step) to ensure the flag reflects the running state.
| else: | ||
| error_context.context("Start Netserver on guest", test.log.info) | ||
| if params.get("os_type") == "windows": | ||
| timeout = float(params.get("timeout", "240")) |
There was a problem hiding this comment.
Remove unused timeout variable.
The variable timeout is assigned but never used. This is dead code.
🔎 Proposed fix
if params.get("os_type") == "windows":
- timeout = float(params.get("timeout", "240"))
cdrom_drv = utils_misc.get_winutils_vol(server_ctl)📝 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.
| timeout = float(params.get("timeout", "240")) | |
| if params.get("os_type") == "windows": | |
| cdrom_drv = utils_misc.get_winutils_vol(server_ctl) |
🧰 Tools
🪛 Ruff (0.14.10)
580-580: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/netperf.py around line 580, the
variable `timeout` is assigned from params but never used; remove the dead
assignment line (timeout = float(params.get("timeout", "240"))) or, if a timeout
was intended, replace its usage in the appropriate function call or loop so the
parsed value is actually passed where needed; ensure no references remain to
`timeout` after removal to avoid unused-variable warnings.
| :return: list of target interfaces(bridges and ports) if return_port is Ture, | ||
| else empty list |
There was a problem hiding this comment.
Fix typo in docstring: "Ture" → "True".
🔎 Proposed fix
- :return: list of target interfaces(bridges and ports) if return_port is Ture,
+ :return: list of target interfaces(bridges and ports) if return_ports is True,📝 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.
| :return: list of target interfaces(bridges and ports) if return_port is Ture, | |
| else empty list | |
| :return: list of target interfaces(bridges and ports) if return_ports is True, | |
| else empty list |
🤖 Prompt for AI Agents
In provider/virtual_network/vdpa_utils.py around lines 101 to 102, the docstring
contains a typo: "Ture" should be "True"; update the docstring to read "if
return_port is True" (and optionally fix surrounding punctuation/spacing for
clarity).
| inf_list_all = _pnpdrv_info(session, device_name, ["InfName"]) | ||
| inf_list = list(set(inf_list_all)) | ||
|
|
||
| # pnputil flags available starting in Windows 10, | ||
| # version 1607, build 14393 later | ||
| build_ver = system.version(session).split(".")[2] | ||
| if int(build_ver) > 14393: | ||
| uninst_store_cmd = "pnputil /delete-driver %s /uninstall /force" % inf_list[0] | ||
| else: | ||
| uninst_store_cmd = "pnputil /f /d %s" % inf_list[0] | ||
| status, output = session.cmd_status_output(uninst_store_cmd, INSTALL_TIMEOUT) |
There was a problem hiding this comment.
Potential IndexError if no matching drivers found.
If _pnpdrv_info returns an empty list (no drivers matching device_name), then inf_list will be empty and accessing inf_list[0] on line 103 will raise an IndexError.
🔎 Proposed fix
inf_list_all = _pnpdrv_info(session, device_name, ["InfName"])
inf_list = list(set(inf_list_all))
+ if not inf_list:
+ test.error("No driver inf files found for device '%s'" % device_name)
# pnputil flags available starting in Windows 10,
# version 1607, build 14393 later🤖 Prompt for AI Agents
In provider/windows/win_driver_utils.py around lines 96 to 106, the code assumes
inf_list has at least one element and will raise IndexError when no matching
drivers are found; add a safe guard after computing inf_list_all/inf_list to
check if inf_list is empty and handle it (either log a warning and skip
uninstallation/return early, or raise a clear, descriptive exception) before
referencing inf_list[0]; ensure the function returns a sensible value or status
in the no-driver case and that the rest of the logic only runs when inf_list is
non-empty.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (14)
provider/virtual_network/netperf_base.py (1)
88-100: Verify output redirection is not duplicated.Past review flagged that the
optionparameter passed tonetperf_threadat line 691 in netperf.py already includes>> %sredirection, but line 98 appends the same redirection again, potentially causing double redirection.#!/bin/bash # Verify the option string construction in netperf.py around line 691 rg -n "option.*>>.*fname" libvirt/tests/src/virtual_network/qemu/netperf.py -A2 -B2Based on past review comments.
libvirt/tests/src/virtual_network/qemu/netperf.py (5)
580-580: Remove unusedtimeoutvariable.Past review correctly identified that the variable
timeoutis assigned but never used. This is dead code.🔎 Proposed fix
if params.get("os_type") == "windows": - timeout = float(params.get("timeout", "240")) cdrom_drv = utils_misc.get_winutils_vol(server_ctl)Based on static analysis hints.
697-702: Bareexceptclause hides errors.Past review correctly identified that the bare
exceptat line 697 catches all exceptions includingKeyboardInterruptandSystemExit, which can mask real issues. Catch specific exceptions or at minimumException.🔎 Proposed fix
def all_clients_up(): try: content = netperf_base.ssh_cmd(clients[-1], "cat %s" % fname) - except: + except Exception: content = "" return FalseBased on static analysis hints.
808-810: Unreachable code afterreturn.Past review correctly identified that the
breakstatement at line 810 is unreachable because it follows areturnstatement. Remove the dead code.🔎 Proposed fix
ret["thu"] = parse_demo_result(fname, int(sessions)) return ret - break else:
174-178: Bug: Loop variable shadows outerdriver_namevariable.Past review correctly identified that the loop at line 174 reuses
driver_nameas the loop variable, overwriting the originaldriver_namefrom line 172. After the loop,driver_namewill be the last value from the split, not the original parameter value.🔎 Proposed fix
media_type = params["virtio_win_media_type"] driver_name = params["driver_name"] session = vm.wait_for_login(nic_index=2, timeout=login_timeout) - for driver_name in driver_name.split(): + for drv_name in driver_name.split(): inf_path = win_driver_utils.get_driver_inf_path( - session, test, media_type, driver_name + session, test, media_type, drv_name ) - if driver_name == "netkvm": + if drv_name == "netkvm":And update all subsequent references within the loop to use
drv_name.Based on static analysis hints.
573-596: Global flag_netserver_startedis checked but never set toTrue.Past review correctly identified that the code checks
_netserver_startedat line 575 but never sets it toTrueafter successfully starting the netserver. This means the netserver start logic runs on every call tolaunch_client.🔎 Proposed fix
Add after line 596:
test.log.info("Netserver start successfully") + _netserver_started = TrueAlso add
global _netserver_startedat the beginning oflaunch_client:def launch_client(...): """Launch netperf clients""" + global _netserver_started netperf_version = params.get("netperf_version", "2.6.0")provider/virtual_network/network_base.py (1)
595-596: Logging bug: incorrect variable logged.Past review correctly identified this issue. The log message uses
vcpu_count(total vCPUs) instead ofvcpu(current vCPU index). This logs the same number for every pinning operation rather than the current vCPU being pinned.🔎 Proposed fix
for vcpu in range(vcpu_count): cpu = cpus[vcpu] vm.vcpupin(vcpu, str(cpu)) - LOG.info("Pinned %d vcpu of vm %s to host cpu %s", - vcpu_count, vm.name, cpu) + LOG.info("Pinned vcpu %d of vm %s to host cpu %s", + vcpu, vm.name, cpu) return vcpu_countprovider/virtual_network/vdpa_utils.py (2)
66-81: Remove unusedovsparameter.Past review correctly identified that the
ovsparameter is declared but never used in the function body. Flow operations useutils_net.openflow_managerdirectly.🔎 Proposed fix
-def add_flows_to_ovs_bridge(bridge, ovs): +def add_flows_to_ovs_bridge(bridge): """ Add flow rules to the given ovs bridge. - :parma bridge: OVS bridge name + :param bridge: OVS bridge name :type bridge: str - :param ovs: OVS instance - :type ovs: OpenVSwitch """Also update the caller at line 125:
- add_flows_to_ovs_bridge(ovs_br, self.ovs) + add_flows_to_ovs_bridge(ovs_br)Based on static analysis hints.
101-102: Fix typo in docstring: "Ture" → "True".Past review correctly identified the typo. Also note the parameter name mismatch: docstring says
return_portbut parameter isreturn_ports.🔎 Proposed fix
- :return: list of target interfaces(bridges and ports) if return_port is Ture, + :return: list of target interfaces(bridges and ports) if return_ports is True,provider/windows/win_driver_utils.py (4)
96-106: PotentialIndexErrorif no matching drivers found.Past review correctly identified that if
_pnpdrv_inforeturns an empty list (no drivers matchingdevice_name), accessinginf_list[0]on line 103 will raise anIndexError.🔎 Proposed fix
inf_list_all = _pnpdrv_info(session, device_name, ["InfName"]) inf_list = list(set(inf_list_all)) + if not inf_list: + test.error("No driver inf files found for device '%s'" % device_name) # pnputil flags available starting in Windows 10, # version 1607, build 14393 later
242-243: PotentialAttributeErrorif version regex doesn't match.Past review correctly identified that if
cdrom_virtio_pathdoesn't match the pattern,matchwill beNone, and callingmatch.group(1)will raise anAttributeError.🔎 Proposed fix
match = re.search(r"virtio-win-(\d+\.\d+(?:\.\d+)?-\d+)", cdrom_virtio_path) + if not match: + test.error("Could not extract version from virtio-win path: %s" % cdrom_virtio_path) cdrom_virtio_version = re.sub("-", ".", match.group(1))
375-378: PotentialIndexErrorif no device found.Past review correctly identified that if
_pnpdrv_inforeturns an empty list (no devices matchingdevice_name), accessingoutput[0]will raise anIndexError.🔎 Proposed fix
output = _pnpdrv_info(session, device_name, ["DeviceID"]) + if not output: + test.fail("No device found matching '%s'" % device_name) # workaround for viostor/vioscsi to get data device id device_id = output[0]
400-404: Bug: Unused return value and incorrect function call.Past review correctly identified that line 401 calls
params.get()but discards the result, and line 404 callsinstall_driver_by_virtio_mediawith only 4 arguments while the function signature at line 159 requires 6 arguments.🔎 Proposed fix
driver_name = params["driver_name"] devcon_path = params["devcon_path"] if load_method != "enable": - params.get("virtio_win_media_type", "iso") - + media_type = params.get("virtio_win_media_type", "iso") device_hwid = driver_info_dict[driver_name]["hwid"] - install_driver_by_virtio_media(session, test, devcon_path, device_hwid) + install_driver_by_virtio_media(session, test, devcon_path, media_type, driver_name, device_hwid) else:libvirt/tests/cfg/virtual_network/qemu/netperf.cfg (1)
99-99: Windows MTU command has mismatched quotes.Past review correctly identified that the command string has an opening
"before%sbut no matching close, which will cause the command to fail on Windows.🔎 Proposed fix
- server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s" + server_mtu_cmd = "netsh interface ipv4 set interface \"%s\" mtu=%s"Or use single quotes for the outer string:
- server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s" + server_mtu_cmd = 'netsh interface ipv4 set interface "%s" mtu=%s'
🧹 Nitpick comments (1)
provider/virtual_network/network_base.py (1)
673-674: Improve exception chaining for better diagnostics.The broad
except Exceptionat line 673 should useraise ... from eto preserve the original exception traceback, aiding debugging.🔎 Proposed fix
except Exception as e: - raise exceptions.TestError("Failed to pin VM threads: %s" % e) + raise exceptions.TestError("Failed to pin VM threads: %s" % e) from eBased on static analysis hints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.pyprovider/virtual_network/vdpa_utils.pyprovider/windows/win_driver_utils.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.py
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/netperf.py
🧬 Code graph analysis (2)
provider/windows/win_driver_utils.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
libvirt/tests/src/virtual_network/qemu/netperf.py (5)
provider/virtual_network/netperf_base.py (1)
ssh_cmd(71-85)provider/virtual_network/vdpa_utils.py (2)
OVSHandler(84-132)get_vdpa_ovs_info(92-132)provider/windows/win_driver_utils.py (2)
get_driver_inf_path(123-155)uninstall_driver(77-120)provider/virtual_network/network_base.py (1)
pin_vcpu_vhost_threads(644-674)libvirt/tests/src/virsh_cmd/network/virsh_net_dhcp_leases.py (1)
f(149-150)
🪛 Ruff (0.14.10)
provider/virtual_network/vdpa_utils.py
16-16: Function call with shell=True parameter identified, security issue
(S604)
33-33: Function call with shell=True parameter identified, security issue
(S604)
49-49: Function call with shell=True parameter identified, security issue
(S604)
63-63: Function call with shell=True parameter identified, security issue
(S604)
66-66: Unused function argument: ovs
(ARG001)
provider/virtual_network/netperf_base.py
43-43: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
47-47: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
80-80: Function call with shell=True parameter identified, security issue
(S604)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
libvirt/tests/src/virtual_network/qemu/netperf.py
99-99: Function call with shell=True parameter identified, security issue
(S604)
264-264: Function call with shell=True parameter identified, security issue
(S604)
269-269: Function call with shell=True parameter identified, security issue
(S604)
295-295: Loop control variable i not used within loop body
(B007)
360-360: Function call with shell=True parameter identified, security issue
(S604)
367-367: Probable insecure usage of temporary file or directory: "/tmp/log_guestinfo.sh"
(S108)
537-537: Function call with shell=True parameter identified, security issue
(S604)
561-561: Ambiguous variable name: l
(E741)
563-563: Unused function argument: port
(ARG001)
570-570: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netperf"
(S108)
571-571: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netserver"
(S108)
580-580: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
620-620: Avoid specifying long messages outside the exception class
(TRY003)
681-681: Unused function argument: timeout
(ARG001)
682-682: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
684-684: Probable insecure usage of temporary file or directory: "/tmp/netperf_agent.py %d %s -D 1 -H %s -l %s %s"
(S108)
697-697: Do not use bare except
(E722)
749-749: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
provider/virtual_network/network_base.py
621-621: Function call with shell=True parameter identified, security issue
(S604)
635-635: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
637-637: Function call with shell=True parameter identified, security issue
(S604)
673-673: Do not catch blind exception: Exception
(BLE001)
674-674: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In @libvirt/tests/cfg/virtual_network/qemu/netperf.cfg:
- Around line 47-53: The netperf configuration sets netperf_version = 2.7.1 but
the Windows startup uses netserver-2.6.0.exe, causing a version mismatch; fix by
making the versions consistent: either change netserver reference to
netserver-2.7.1.exe wherever netserver-2.6.0.exe is referenced, or lower
netperf_version to 2.6.0 so both use the same release; update the setup_cmds
(and any platform-specific ppc64/ppc64le setup_cmd variants) and the Windows
netserver filename to match the chosen single version and ensure any
packaging/ISO references reflect that same version.
- Around line 33-44: Replace hard-coded credentials (username_client,
password_client, username_host, password_host) with non-sensitive placeholders
or runtime-configurable secrets; for example, use placeholder values like
"<USERNAME>" and "<PASSWORD>", read from environment variables, or load from a
secret manager in the code that consumes this cfg. Ensure shell_prompt_* and
other test values remain unchanged, and update any test harness or README to
document the required env vars or secret keys so developers supply real
credentials only in secure, non-committed configuration.
- Around line 70-85: The server_mtu_cmd string is broken by unescaped inner
double quotes: update the server_mtu_cmd assignment so the inner quotes around
%s are escaped or use a different quoting style so the final value is a single
valid string (preserve the %s placeholders for interface and mtu); modify the
server_mtu_cmd entry accordingly (refer to the server_mtu_cmd symbol in this
diff).
In @libvirt/tests/src/virtual_network/qemu/netperf.py:
- Around line 28-29: The flag _netserver_started is never updated after
launching netserver, so add code to set _netserver_started = True immediately
after a successful start (after the subprocess/process start call in the
netserver startup block) and ensure the function updates the module-level
variable (declare global _netserver_started in that function if needed) so
subsequent checks (the _netserver_started gating used later) will skip
restarting netserver.
- Around line 743-751: The function all_clients_up() currently uses a bare
except around netperf_base.ssh_cmd(clients[-1], "cat %s" % fname); replace the
bare except with catching the expected exception types (e.g., the
library-specific remote command failure or SSH-related exceptions) or at minimum
use "except Exception as e" and log the error at debug including the exception
and context (clients[-1], fname) before returning False; ensure the rest of the
logic using sessions and re.findall("MIGRATE", content) remains unchanged.
- Around line 135-158: The Windows NIC/driver loop has index and variable
mixups: change vm.wait_for_login(nic_index=2, ...) to nic_index=1 for the second
NIC (indices are 0 and 1), and inside the driver loop use the loop variable
consistently—for drv_name in driver_name.split(): replace any checks of
driver_name == "VIOPROT" with drv_name == "VIOPROT", and pass drv_name (not the
full driver_name string) to uninstall_driver and install_driver calls so each
individual driver is handled correctly.
- Around line 440-455: The function start_test currently accepts test=None but
uses test unconditionally and will crash; make test a required parameter by
removing the default None in start_test(...) so its signature is def
start_test(..., params=None, test): (i.e., no default) and update any callers to
pass a proper test object, or alternatively add an explicit guard at the start
of start_test that raises a clear exception (e.g., raise ValueError("test must
be provided")) if test is None; adjust references where test is used (e.g., the
usages noted around lines 477, 513, 575) accordingly.
In @provider/virtual_network/netperf_base.py:
- Line 53: Fix the typo in the docstring/comment that currently reads "Tweak
configuration with truned profile" by replacing "truned" with "tuned" so the
line reads "Tweak configuration with tuned profile"; locate the string in
provider/virtual_network/netperf_base.py (search for "truned") and update the
docstring/comment accordingly, preserving surrounding punctuation and
formatting.
- Around line 94-96: Validate and sanitize the NUMA node value before building
the numactl command: fetch params.get("numa_node") with a safe default or check
for None, parse it with int inside a try/except to catch ValueError/TypeError,
and ensure it is non-negative; remove the unconditional abs() and only subtract
1 if you explicitly expect 1-based input (in which case require parsed value >=
1 and then compute node_index = value - 1), otherwise use the parsed value
directly; if validation fails, disable NUMA (numa_enable=False) or log/raise a
clear error instead of constructing an invalid numactl argument in the code that
builds cmd.
- Line 44: The call to ssh_cmd(session, params.get("setup_cmd")) can pass None
and cause a runtime error; add a presence check for params.get("setup_cmd")
before invoking ssh_cmd (same pattern used for env_setup_cmd) so ssh_cmd is only
called when a non-None setup_cmd exists, or alternatively enforce that all
callers populate setup_cmd in params; update the env_setup invocation around
ssh_cmd to guard with if params.get("setup_cmd"): ssh_cmd(...)
In @provider/virtual_network/network_base.py:
- Around line 658-675: pin_vcpus() and pin_vcpu_vhost_threads() can raise
obscure IndexError/TypeError on small NUMA nodes or platforms where
os.cpu_count() is None; validate inputs and add clear error handling: in
pin_vcpus(), ensure cpus is a sequence of sufficient length (or compute n =
min(vcpu_count, len(cpus))) and coerce/validate each cpu id is an int before
calling vm.vcpupin, wrap vm.vcpupin in try/except to log a contextual error
including vm.name, vcpu index and cpu value and re-raise or return a meaningful
error/count; in pin_vcpu_vhost_threads(), call os.cpu_count() and fall back to 1
(or a provided default) if it returns None, and validate any CPU list lengths
similarly, logging context on failures; reference pin_vcpus,
pin_vcpu_vhost_threads, vm.vcpupin and os.cpu_count when locating the changes.
In @provider/virtual_network/vdpa_utils.py:
- Around line 9-34: The functions get_vf_pci_address, get_pf_pci_address and
get_pf_port currently interpolate nic/pci identifiers into a shell pipeline via
.format(...) which is an injection vector; change these callers to avoid shell
interpolation by invoking the commands with a safe API (e.g.,
subprocess.run/subprocess.check_output with a list of args) or at minimum
escape/quote the variable (shlex.quote) before inserting it, and pass
shell=False to process.system/process.system_output or use safe grep flags (grep
-F or -- fixed-strings) with the identifier as an argument rather than embedding
it inside a single shell string; update the calls that build cmd in those
functions so the identifier is passed as a separate argument and remove
shell=True to eliminate command injection risk.
- Around line 66-82: The function add_flows_to_ovs_bridge(bridge, ovs) currently
ignores the ovs parameter; either remove the unused parameter from the signature
and update all callers to call add_flows_to_ovs_bridge(bridge) or route the
openflow operations through the provided ovs instance by replacing
utils_net.openflow_manager(...) calls with calls on the ovs object (e.g.,
ovs.openflow_manager(...) or ovs.run_command(...) depending on the ovs API) so
the passed instance is actually used; ensure callers and any docstring/typing
are updated accordingly.
In @provider/windows/win_driver_utils.py:
- Around line 389-410: The call to install_driver_by_virtio_media in load_driver
is missing arguments and will raise a TypeError; retrieve the virtio media type
into a variable (virtio_media = params.get("virtio_win_media_type", "iso")),
then call install_driver_by_virtio_media(session, test, devcon_path,
virtio_media, driver_name, device_hwid) so the function receives (session, test,
devcon_path, media_type, driver_name, device_hwid) as required; update the
non-enable branch of load_driver accordingly.
- Around line 218-265: In run_installer(), guard the regex result before using
match.group(1): check if match is None and either fail the test with a clear
message (e.g., test.fail("Unexpected virtio ISO name: {cdrom_virtio_path}")) or
derive a safe default for cdrom_virtio_version; then continue. Also add a
timeout to the installer invocation by calling session.cmd(run_installer_cmd,
timeout=<reasonable_seconds>) instead of session.cmd(run_installer_cmd) so the
call doesn't block forever. Update references: the match variable from
re.search(...), cdrom_virtio_version assignment, and the
session.cmd(run_installer_cmd) call.
- Around line 77-120: The uninstall_driver function currently uses inf_list[0]
without checking for an empty list and uses a strict > 14393 check that excludes
build 14393; update uninstall_driver to compute inf_list =
list(set(_pnpdrv_info(session, device_name, ["InfName"]))) then if not inf_list
call test.error (or return) with a clear message that no INF was found (include
device_name/device_hwid/output context) to avoid an IndexError, and change the
pnputil condition from if int(build_ver) > 14393 to if int(build_ver) >= 14393
so build 14393 uses the newer flags; reference symbols: uninstall_driver,
_pnpdrv_info, inf_list, build_ver, pnputil.
🧹 Nitpick comments (3)
provider/virtual_network/vdpa_utils.py (1)
92-132: Dedupetarget_ifacesand guard vdpa bridge discovery failures
target_ifacescan contain duplicates (ports + bridge across multiple NICs), and the vdpa branch doesn’t catchfind_current_bridge()failures (Lines 113-118).Proposed fix
def get_vdpa_ovs_info(self, add_flows=True, return_ports=True): @@ - target_ifaces = [] + target_ifaces = [] @@ if nic.nettype == "vdpa": @@ - manager, ovs_br = utils_net.find_current_bridge(port) + try: + manager, ovs_br = utils_net.find_current_bridge(port) + except Exception: + ovs_br = None @@ - return target_ifaces + # preserve order while deduping + return list(dict.fromkeys(target_ifaces))libvirt/tests/src/virtual_network/qemu/netperf.py (1)
104-110: Reduceshell=Trueusage for param-derived commands (or sanitize inputs)Several calls run
shell=Truewith strings partially sourced from params (e.g.,host_mtu_cmd, scripts, rm commands). Even in tests, this is an avoidable footgun if someone copies configs around.Also applies to: 308-316, 404-406, 585-591
provider/virtual_network/netperf_base.py (1)
21-27: Consider caching SSH command results to avoid redundant calls.Lines 21-23 and 26-27 execute the same SSH commands twice. Storing the results in variables would improve efficiency.
♻️ Proposed refactor to cache results
+ kvm_userspace_ver = ssh_cmd(host, ver_cmd).strip() + guest_kernel_ver = ssh_cmd(server_ctl, guest_ver_cmd).strip() + - test.write_test_keyval({"kvm-userspace-ver": ssh_cmd(host, ver_cmd).strip()}) + test.write_test_keyval({"kvm-userspace-ver": kvm_userspace_ver}) test.write_test_keyval( - {"guest-kernel-ver": ssh_cmd(server_ctl, guest_ver_cmd).strip()} + {"guest-kernel-ver": guest_kernel_ver} ) test.write_test_keyval({"session-length": test_duration}) - fd.write("### kvm-userspace-ver : %s\n" % ssh_cmd(host, ver_cmd).strip()) - fd.write("### guest-kernel-ver : %s\n" % ssh_cmd(server_ctl, guest_ver_cmd).strip()) + fd.write("### kvm-userspace-ver : %s\n" % kvm_userspace_ver) + fd.write("### guest-kernel-ver : %s\n" % guest_kernel_ver)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.pyprovider/virtual_network/vdpa_utils.pyprovider/windows/win_driver_utils.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.py
📚 Learning: 2026-01-07T16:04:00.667Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6754
File: libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_operation.cfg:9-10
Timestamp: 2026-01-07T16:04:00.667Z
Learning: In tp-libvirt test configurations, parameters like `master_images_clone` and `remove_image_image1` are processed by the external avocado-vt/virttest framework's image preprocessing logic, not by test files within the tp-libvirt repository itself.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/netperf.py
🧬 Code graph analysis (2)
provider/windows/win_driver_utils.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
provider/virtual_network/network_base.py (1)
libvirt/tests/src/virtual_network/qemu/netperf.py (1)
run(47-435)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/netperf.py
104-104: Function call with shell=True parameter identified, security issue
(S604)
135-135: Use of possibly insecure function; consider using ast.literal_eval
(S307)
153-153: Use of possibly insecure function; consider using ast.literal_eval
(S307)
308-308: Function call with shell=True parameter identified, security issue
(S604)
313-313: Function call with shell=True parameter identified, security issue
(S604)
339-339: Loop control variable i not used within loop body
(B007)
404-404: Function call with shell=True parameter identified, security issue
(S604)
411-411: Probable insecure usage of temporary file or directory: "/tmp/log_guestinfo.sh"
(S108)
586-586: Function call with shell=True parameter identified, security issue
(S604)
610-610: Ambiguous variable name: l
(E741)
612-612: Unused function argument: port
(ARG001)
619-619: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netperf"
(S108)
620-620: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netserver"
(S108)
629-629: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
669-669: Avoid specifying long messages outside the exception class
(TRY003)
730-730: Unused function argument: timeout
(ARG001)
731-731: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
733-733: Probable insecure usage of temporary file or directory: "/tmp/netperf_agent.py %d %s -D 1 -H %s -l %s %s"
(S108)
746-746: Do not use bare except
(E722)
798-798: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
provider/virtual_network/vdpa_utils.py
16-16: Function call with shell=True parameter identified, security issue
(S604)
33-33: Function call with shell=True parameter identified, security issue
(S604)
49-49: Function call with shell=True parameter identified, security issue
(S604)
63-63: Function call with shell=True parameter identified, security issue
(S604)
66-66: Unused function argument: ovs
(ARG001)
provider/virtual_network/netperf_base.py
43-43: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
47-47: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
80-80: Function call with shell=True parameter identified, security issue
(S604)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
provider/virtual_network/network_base.py
699-699: Function call with shell=True parameter identified, security issue
(S604)
713-713: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
715-715: Function call with shell=True parameter identified, security issue
(S604)
751-751: Do not catch blind exception: Exception
(BLE001)
752-752: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (6)
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg (1)
1-4: Fix cfg parsing errors: broken Windows quoting and structure issuesLine 74 has unescaped quotes that will break cfg parsing:
"netsh interface ipv4 set interface "%s" mtu=%s"should escape the inner quotes. Additionally, the file has twovariants:blocks at the same indentation level (lines 92 and 130), which may cause the parser to ignore one or fail parsing entirely—verify whether this duplicated structure is intentional or should be consolidated.Also present: plaintext passwords at lines 34 and 40 (
password_client = 123456,password_host = 123456); netserv_start_cmd referencesnetserver-2.6.0.exewhile netperf_version is set to 2.7.1 (lines 47 and 101).Also applies to: 92-163
⛔ Skipped due to learnings
Learnt from: yanglei-rh Repo: autotest/tp-libvirt PR: 6754 File: libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_operation.cfg:9-10 Timestamp: 2026-01-07T16:04:00.667Z Learning: In tp-libvirt test configurations, parameters like `master_images_clone` and `remove_image_image1` are processed by the external avocado-vt/virttest framework's image preprocessing logic, not by test files within the tp-libvirt repository itself.Learnt from: qiankehan Repo: autotest/tp-libvirt PR: 6652 File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106 Timestamp: 2025-11-24T10:44:47.801Z Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.provider/virtual_network/network_base.py (1)
694-701: Harden shell usage and conditionally adoptzip(..., strict=...)if Python 3.10+ is supportedThe
shell=Trueat line 699 follows established patterns in the codebase but hardening could improve security. At line 713, consider addingstrict=Trueto thezip()call to catch length mismatches—this requires Python 3.10+. Verify the project's minimum Python version before implementing.provider/virtual_network/netperf_base.py (4)
1-9: LGTM!The imports are appropriate for netperf testing utilities and all are utilized within the module.
79-82: Verify that commands executed withshell=Truecome from trusted sources.Using
shell=Truecan be a security risk if thecmdparameter contains untrusted input. Ensure all commands passed to this function originate from test configuration files and not from external or user-controlled sources.Based on the PR objectives, this code is migrated from tp-qemu where it's been in use, so this is likely acceptable in the controlled test environment.
103-119: LGTM!The formatting logic correctly handles strings, integers, and floats with appropriate type checking. The static analysis warning about the exception message (TRY003) is pedantic and can be safely ignored in this context.
122-147: LGTM!The function correctly builds pipe-delimited records with optional headers, maintaining consistent formatting through the
format_resulthelper.
| username_client = root | ||
| password_client = 123456 | ||
| shell_client_client = ssh | ||
| shell_port_client = 22 | ||
| shell_prompt_client = \[root@.{0,50}][\#\$] | ||
| #host configuration | ||
| shell_port_host = 22 | ||
| password_host = 123456 | ||
| username_host = root | ||
| os_type_client = linux | ||
| os_type_host = linux | ||
| shell_prompt_host = \[root@.{0,50}][\#\$] |
There was a problem hiding this comment.
Avoid committing real credentials in cfg
username_* = root and password_* = 123456 (Lines 33-41) are easy to accidentally reuse outside a lab context.
Also applies to: 40-41
🤖 Prompt for AI Agents
In @libvirt/tests/cfg/virtual_network/qemu/netperf.cfg around lines 33 - 44,
Replace hard-coded credentials (username_client, password_client, username_host,
password_host) with non-sensitive placeholders or runtime-configurable secrets;
for example, use placeholder values like "<USERNAME>" and "<PASSWORD>", read
from environment variables, or load from a secret manager in the code that
consumes this cfg. Ensure shell_prompt_* and other test values remain unchanged,
and update any test harness or README to document the required env vars or
secret keys so developers supply real credentials only in secure, non-committed
configuration.
| netperf_version = 2.7.1 | ||
| netperf_pkg = netperf/netperf-2.7.1.tar.bz2 | ||
| setup_cmd = "cd /tmp && rm -rf netperf-2.7.1 && tar xvfj netperf-2.7.1.tar.bz2 && cd netperf-2.7.1 && sh autogen.sh && CFLAGS=-Wno-implicit-function-declaration ./configure --enable-burst --enable-demo=yes && make" | ||
| ppc64: | ||
| setup_cmd = "cd /tmp && rm -rf netperf-2.7.1 && tar xvfj netperf-2.7.1.tar.bz2 && cd netperf-2.7.1 && sh autogen.sh && CFLAGS=-Wno-implicit-function-declaration ./configure --build=ppc64 --enable-burst --enable-demo=yes && make" | ||
| ppc64le: | ||
| setup_cmd = "cd /tmp && rm -rf netperf-2.7.1 && tar xvfj netperf-2.7.1.tar.bz2 && cd netperf-2.7.1 && sh autogen.sh && CFLAGS=-Wno-implicit-function-declaration ./configure --build=ppc64le --enable-burst --enable-demo=yes && make" |
There was a problem hiding this comment.
Netserver version mismatch vs netperf version
Config sets netperf_version = 2.7.1 (Line 47) but Windows starts netserver-2.6.0.exe (Line 101). If the ISO/deps only provide one version, this will fail.
Also applies to: 101-103
🤖 Prompt for AI Agents
In @libvirt/tests/cfg/virtual_network/qemu/netperf.cfg around lines 47 - 53, The
netperf configuration sets netperf_version = 2.7.1 but the Windows startup uses
netserver-2.6.0.exe, causing a version mismatch; fix by making the versions
consistent: either change netserver reference to netserver-2.7.1.exe wherever
netserver-2.6.0.exe is referenced, or lower netperf_version to 2.6.0 so both use
the same release; update the setup_cmds (and any platform-specific ppc64/ppc64le
setup_cmd variants) and the Windows netserver filename to match the chosen
single version and ensure any packaging/ISO references reflect that same
version.
| Windows: | ||
| # log_guestinfo_script = scripts/rh_perf_log_guestinfo_script.bat | ||
| # log_guestinfo_exec = cmd /c | ||
| # log_guestinfo_path = C:\log_guestinfo.bat | ||
| server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s" | ||
| i386, x86_64: | ||
| cpu_model_flags = ",hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0xfff" | ||
| windows_disable_firewall = "netsh advfirewall set allprofiles state off" | ||
| devcon_dirname = "win7_" | ||
| i386: | ||
| devcon_dirname += "x86" | ||
| x86_64: | ||
| devcon_dirname += "amd64" | ||
| client_mtu_cmd = "ifconfig %s mtu %s" | ||
| host_mtu_cmd = "ifconfig %s mtu %s" | ||
| #FIXME: it's a workaround, it needs a better way to disable all the firewall rules |
There was a problem hiding this comment.
Fix Windows server_mtu_cmd quoting (likely broken string)
Line 74: server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s" will terminate the string early. It should escape inner quotes.
Proposed fix
- server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s"
+ server_mtu_cmd = "netsh interface ipv4 set interface \"%s\" mtu=%s"📝 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.
| Windows: | |
| # log_guestinfo_script = scripts/rh_perf_log_guestinfo_script.bat | |
| # log_guestinfo_exec = cmd /c | |
| # log_guestinfo_path = C:\log_guestinfo.bat | |
| server_mtu_cmd = "netsh interface ipv4 set interface "%s" mtu=%s" | |
| i386, x86_64: | |
| cpu_model_flags = ",hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0xfff" | |
| windows_disable_firewall = "netsh advfirewall set allprofiles state off" | |
| devcon_dirname = "win7_" | |
| i386: | |
| devcon_dirname += "x86" | |
| x86_64: | |
| devcon_dirname += "amd64" | |
| client_mtu_cmd = "ifconfig %s mtu %s" | |
| host_mtu_cmd = "ifconfig %s mtu %s" | |
| #FIXME: it's a workaround, it needs a better way to disable all the firewall rules | |
| Windows: | |
| # log_guestinfo_script = scripts/rh_perf_log_guestinfo_script.bat | |
| # log_guestinfo_exec = cmd /c | |
| # log_guestinfo_path = C:\log_guestinfo.bat | |
| server_mtu_cmd = "netsh interface ipv4 set interface \"%s\" mtu=%s" | |
| i386, x86_64: | |
| cpu_model_flags = ",hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0xfff" | |
| windows_disable_firewall = "netsh advfirewall set allprofiles state off" | |
| devcon_dirname = "win7_" | |
| i386: | |
| devcon_dirname += "x86" | |
| x86_64: | |
| devcon_dirname += "amd64" | |
| client_mtu_cmd = "ifconfig %s mtu %s" | |
| host_mtu_cmd = "ifconfig %s mtu %s" | |
| #FIXME: it's a workaround, it needs a better way to disable all the firewall rules |
🤖 Prompt for AI Agents
In @libvirt/tests/cfg/virtual_network/qemu/netperf.cfg around lines 70 - 85, The
server_mtu_cmd string is broken by unescaped inner double quotes: update the
server_mtu_cmd assignment so the inner quotes around %s are escaped or use a
different quoting style so the final value is a single valid string (preserve
the %s placeholders for interface and mtu); modify the server_mtu_cmd entry
accordingly (refer to the server_mtu_cmd symbol in this diff).
| _netserver_started = False | ||
|
|
There was a problem hiding this comment.
Set _netserver_started = True after starting netserver
_netserver_started is checked (Lines 624-626) but never set, so you’ll try to start netserver every time.
Proposed fix
def launch_client(
@@
global _netserver_started
@@
- if _netserver_started:
+ if _netserver_started:
test.log.debug("Netserver already started.")
else:
@@
test.log.info("Netserver start successfully")
+ _netserver_started = TrueAlso applies to: 603-646
🤖 Prompt for AI Agents
In @libvirt/tests/src/virtual_network/qemu/netperf.py around lines 28 - 29, The
flag _netserver_started is never updated after launching netserver, so add code
to set _netserver_started = True immediately after a successful start (after the
subprocess/process start call in the netserver startup block) and ensure the
function updates the module-level variable (declare global _netserver_started in
that function if needed) so subsequent checks (the _netserver_started gating
used later) will skip restarting netserver.
| params["qemu_command_prefix"] = f"numactl -m {pinned_node}" | ||
| vm_name = params["main_vm"] | ||
| use_vmxml = params.get('use_vmxml', 'yes') == 'yes' | ||
| backup_vmxml = None | ||
| if use_vmxml: | ||
| vm = env.get_vm(vm_name) | ||
| if vm is None: | ||
| env.create_vm(vm_type='libvirt', target=None, name=vm_name, params=params, bindir=test.bindir) | ||
| vm = env.get_vm(vm_name) | ||
|
|
||
| backup_vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | ||
| vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | ||
|
|
||
| vm_attrs = eval(params.get('vm_attrs', '{}')) | ||
| if vm_attrs: | ||
| vmxml.setup_attrs(**vm_attrs) | ||
|
|
||
| # Remove all interfaces and rebuild from scratch | ||
| vmxml.del_device('interface', by_tag=True) | ||
|
|
||
| # Configure primary interface (NIC 0) | ||
| iface_dict = interface_base.parse_iface_dict(params) | ||
| if 'mac_address' not in iface_dict: | ||
| desired_mac = params.get('mac_nic1') or params.get('mac_addr') | ||
| if desired_mac: | ||
| iface_dict['mac_address'] = desired_mac.strip() | ||
| else: | ||
| iface_dict['mac_address'] = utils_net.generate_mac_address_simple() | ||
| libvirt_vmxml.modify_vm_device(vmxml, "interface", iface_dict, 0) | ||
|
|
||
| # Configure second interface (NIC 1) if provided | ||
| iface_dict2 = eval(params.get("iface_dict2", "{}")) | ||
| if iface_dict2: | ||
| if 'mac_address' not in iface_dict2: | ||
| iface_dict2['mac_address'] = utils_net.generate_mac_address_simple() | ||
| libvirt_vmxml.modify_vm_device(vmxml, "interface", iface_dict2, 1) | ||
|
|
||
| vmxml.sync() |
There was a problem hiding this comment.
Replace eval() on params with ast.literal_eval()
vm_attrs = eval(...) (Line 135) and iface_dict2 = eval(...) (Line 153) will execute arbitrary code if params are ever influenced outside trusted cfg.
Proposed fix
import logging
import os
import re
import threading
import time
+import ast
@@
- vm_attrs = eval(params.get('vm_attrs', '{}'))
+ vm_attrs = ast.literal_eval(params.get("vm_attrs", "{}"))
@@
- iface_dict2 = eval(params.get("iface_dict2", "{}"))
+ iface_dict2 = ast.literal_eval(params.get("iface_dict2", "{}"))📝 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.
| params["qemu_command_prefix"] = f"numactl -m {pinned_node}" | |
| vm_name = params["main_vm"] | |
| use_vmxml = params.get('use_vmxml', 'yes') == 'yes' | |
| backup_vmxml = None | |
| if use_vmxml: | |
| vm = env.get_vm(vm_name) | |
| if vm is None: | |
| env.create_vm(vm_type='libvirt', target=None, name=vm_name, params=params, bindir=test.bindir) | |
| vm = env.get_vm(vm_name) | |
| backup_vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| vm_attrs = eval(params.get('vm_attrs', '{}')) | |
| if vm_attrs: | |
| vmxml.setup_attrs(**vm_attrs) | |
| # Remove all interfaces and rebuild from scratch | |
| vmxml.del_device('interface', by_tag=True) | |
| # Configure primary interface (NIC 0) | |
| iface_dict = interface_base.parse_iface_dict(params) | |
| if 'mac_address' not in iface_dict: | |
| desired_mac = params.get('mac_nic1') or params.get('mac_addr') | |
| if desired_mac: | |
| iface_dict['mac_address'] = desired_mac.strip() | |
| else: | |
| iface_dict['mac_address'] = utils_net.generate_mac_address_simple() | |
| libvirt_vmxml.modify_vm_device(vmxml, "interface", iface_dict, 0) | |
| # Configure second interface (NIC 1) if provided | |
| iface_dict2 = eval(params.get("iface_dict2", "{}")) | |
| if iface_dict2: | |
| if 'mac_address' not in iface_dict2: | |
| iface_dict2['mac_address'] = utils_net.generate_mac_address_simple() | |
| libvirt_vmxml.modify_vm_device(vmxml, "interface", iface_dict2, 1) | |
| vmxml.sync() | |
| params["qemu_command_prefix"] = f"numactl -m {pinned_node}" | |
| vm_name = params["main_vm"] | |
| use_vmxml = params.get('use_vmxml', 'yes') == 'yes' | |
| backup_vmxml = None | |
| if use_vmxml: | |
| vm = env.get_vm(vm_name) | |
| if vm is None: | |
| env.create_vm(vm_type='libvirt', target=None, name=vm_name, params=params, bindir=test.bindir) | |
| vm = env.get_vm(vm_name) | |
| backup_vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| vm_attrs = ast.literal_eval(params.get("vm_attrs", "{}")) | |
| if vm_attrs: | |
| vmxml.setup_attrs(**vm_attrs) | |
| # Remove all interfaces and rebuild from scratch | |
| vmxml.del_device('interface', by_tag=True) | |
| # Configure primary interface (NIC 0) | |
| iface_dict = interface_base.parse_iface_dict(params) | |
| if 'mac_address' not in iface_dict: | |
| desired_mac = params.get('mac_nic1') or params.get('mac_addr') | |
| if desired_mac: | |
| iface_dict['mac_address'] = desired_mac.strip() | |
| else: | |
| iface_dict['mac_address'] = utils_net.generate_mac_address_simple() | |
| libvirt_vmxml.modify_vm_device(vmxml, "interface", iface_dict, 0) | |
| # Configure second interface (NIC 1) if provided | |
| iface_dict2 = ast.literal_eval(params.get("iface_dict2", "{}")) | |
| if iface_dict2: | |
| if 'mac_address' not in iface_dict2: | |
| iface_dict2['mac_address'] = utils_net.generate_mac_address_simple() | |
| libvirt_vmxml.modify_vm_device(vmxml, "interface", iface_dict2, 1) | |
| vmxml.sync() |
🧰 Tools
🪛 Ruff (0.14.10)
135-135: Use of possibly insecure function; consider using ast.literal_eval
(S307)
153-153: Use of possibly insecure function; consider using ast.literal_eval
(S307)
| def check_ovs_status(): | ||
| """ | ||
| Check if ovs-vsctl and openvswitch service are installed and running. | ||
| :return: True if both are available and running, otherwise False | ||
| :rtype: bool | ||
| """ | ||
| cmd = "which ovs-vsctl && systemctl status openvswitch.service" | ||
| return process.system(cmd, ignore_status=True, shell=True) == 0 | ||
|
|
||
|
|
||
| def get_vf_pci_address(nic_netdst): | ||
| """ | ||
| Get vf pci address from a given network destination. | ||
| :param nic_netdst: Network destination address | ||
| :type nic_netdst: str | ||
| :return: VF pci address | ||
| :rtype: str | ||
| """ | ||
| cmd = ( | ||
| "vdpa dev show | grep {0} | grep -o 'pci/[^[:space:]]*' | " | ||
| "awk -F/ '{{print $2}}'" | ||
| ).format(nic_netdst) | ||
| return process.system_output(cmd, shell=True).decode().strip() | ||
|
|
There was a problem hiding this comment.
Avoid interpolating interface/PCI identifiers into shell pipelines
get_vf_pci_address(), get_pf_pci_address(), get_pf_port() build shell pipelines with .format(...) (Lines 29-33, 46-50, 62-64). If these inputs ever come from params/cfg, they’re a trivial injection vector.
Proposed hardening (minimal)
import logging
+import shlex
@@
def get_vf_pci_address(nic_netdst):
@@
- cmd = (
- "vdpa dev show | grep {0} | grep -o 'pci/[^[:space:]]*' | "
- "awk -F/ '{{print $2}}'"
- ).format(nic_netdst)
+ safe = shlex.quote(nic_netdst)
+ cmd = (
+ f"vdpa dev show | grep -F {safe} | grep -o 'pci/[^[:space:]]*' | "
+ "awk -F/ '{print $2}'"
+ )
return process.system_output(cmd, shell=True).decode().strip()Also applies to: 36-64
🧰 Tools
🪛 Ruff (0.14.10)
16-16: Function call with shell=True parameter identified, security issue
(S604)
33-33: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
In @provider/virtual_network/vdpa_utils.py around lines 9 - 34, The functions
get_vf_pci_address, get_pf_pci_address and get_pf_port currently interpolate
nic/pci identifiers into a shell pipeline via .format(...) which is an injection
vector; change these callers to avoid shell interpolation by invoking the
commands with a safe API (e.g., subprocess.run/subprocess.check_output with a
list of args) or at minimum escape/quote the variable (shlex.quote) before
inserting it, and pass shell=False to process.system/process.system_output or
use safe grep flags (grep -F or -- fixed-strings) with the identifier as an
argument rather than embedding it inside a single shell string; update the calls
that build cmd in those functions so the identifier is passed as a separate
argument and remove shell=True to eliminate command injection risk.
| def add_flows_to_ovs_bridge(bridge, ovs): | ||
| """ | ||
| Add flow rules to the given ovs bridge. | ||
| :parma bridge: OVS bridge name | ||
| :type bridge: str | ||
| :param ovs: OVS instance | ||
| :type ovs: OpenVSwitch | ||
| """ | ||
| utils_net.openflow_manager( | ||
| bridge, "add-flow", flow_options="in_port=1,idle_timeout=0,actions=output:2" | ||
| ) | ||
| utils_net.openflow_manager( | ||
| bridge, "add-flow", flow_options="in_port=2,idle_timeout=0,actions=output:1" | ||
| ) | ||
| utils_net.openflow_manager(bridge, "dump-flows") | ||
|
|
There was a problem hiding this comment.
Remove unused ovs parameter or use it
add_flows_to_ovs_bridge(bridge, ovs) doesn’t use ovs (Line 66), so either drop it or route flow ops through the passed instance.
Proposed fix
-def add_flows_to_ovs_bridge(bridge, ovs):
+def add_flows_to_ovs_bridge(bridge):
@@
- utils_net.openflow_manager(bridge, "dump-flows")
+ utils_net.openflow_manager(bridge, "dump-flows")📝 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 add_flows_to_ovs_bridge(bridge, ovs): | |
| """ | |
| Add flow rules to the given ovs bridge. | |
| :parma bridge: OVS bridge name | |
| :type bridge: str | |
| :param ovs: OVS instance | |
| :type ovs: OpenVSwitch | |
| """ | |
| utils_net.openflow_manager( | |
| bridge, "add-flow", flow_options="in_port=1,idle_timeout=0,actions=output:2" | |
| ) | |
| utils_net.openflow_manager( | |
| bridge, "add-flow", flow_options="in_port=2,idle_timeout=0,actions=output:1" | |
| ) | |
| utils_net.openflow_manager(bridge, "dump-flows") | |
| def add_flows_to_ovs_bridge(bridge): | |
| """ | |
| Add flow rules to the given ovs bridge. | |
| :parma bridge: OVS bridge name | |
| :type bridge: str | |
| :param ovs: OVS instance | |
| :type ovs: OpenVSwitch | |
| """ | |
| utils_net.openflow_manager( | |
| bridge, "add-flow", flow_options="in_port=1,idle_timeout=0,actions=output:2" | |
| ) | |
| utils_net.openflow_manager( | |
| bridge, "add-flow", flow_options="in_port=2,idle_timeout=0,actions=output:1" | |
| ) | |
| utils_net.openflow_manager(bridge, "dump-flows") |
🧰 Tools
🪛 Ruff (0.14.10)
66-66: Unused function argument: ovs
(ARG001)
🤖 Prompt for AI Agents
In @provider/virtual_network/vdpa_utils.py around lines 66 - 82, The function
add_flows_to_ovs_bridge(bridge, ovs) currently ignores the ovs parameter; either
remove the unused parameter from the signature and update all callers to call
add_flows_to_ovs_bridge(bridge) or route the openflow operations through the
provided ovs instance by replacing utils_net.openflow_manager(...) calls with
calls on the ovs object (e.g., ovs.openflow_manager(...) or ovs.run_command(...)
depending on the ovs API) so the passed instance is actually used; ensure
callers and any docstring/typing are updated accordingly.
| def uninstall_driver(session, test, devcon_path, driver_name, device_name, device_hwid): | ||
| """ | ||
| Uninstall driver. | ||
| :param session: The guest session object. | ||
| :param test: kvm test object | ||
| :param devcon_path: devcon.exe path. | ||
| :param driver_name: driver name. | ||
| :param device_name: device name. | ||
| :param device_hwid: device hardware id. | ||
| """ | ||
| devcon_path = utils_misc.set_winutils_letter(session, devcon_path) | ||
| status, output = session.cmd_status_output( | ||
| "dir %s" % devcon_path, timeout=OPERATION_TIMEOUT | ||
| ) | ||
| if status: | ||
| test.error("Not found devcon.exe, details: %s" % output) | ||
| LOG_JOB.info("Uninstalling previous installed driver") | ||
| # find the inf name and remove the repeated one | ||
| inf_list_all = _pnpdrv_info(session, device_name, ["InfName"]) | ||
| inf_list = list(set(inf_list_all)) | ||
|
|
||
| # pnputil flags available starting in Windows 10, | ||
| # version 1607, build 14393 later | ||
| build_ver = system.version(session).split(".")[2] | ||
| if int(build_ver) > 14393: | ||
| uninst_store_cmd = "pnputil /delete-driver %s /uninstall /force" % inf_list[0] | ||
| else: | ||
| uninst_store_cmd = "pnputil /f /d %s" % inf_list[0] | ||
| status, output = session.cmd_status_output(uninst_store_cmd, INSTALL_TIMEOUT) | ||
| # for viostor and vioscsi, they need system reboot | ||
| # acceptable status: OK(0), REBOOT(3010) | ||
| if status not in (0, 3010): | ||
| test.error( | ||
| "Failed to uninstall driver '%s' from store, " | ||
| "details:\n%s" % (driver_name, output) | ||
| ) | ||
| uninst_cmd = "%s remove %s" % (devcon_path, device_hwid) | ||
| status, output = session.cmd_status_output(uninst_cmd, INSTALL_TIMEOUT) | ||
| # acceptable status: OK(0), REBOOT(1) | ||
| if status > 1: | ||
| test.error( | ||
| "Failed to uninstall driver '%s', details:\n%s" % (driver_name, output) | ||
| ) |
There was a problem hiding this comment.
Handle empty INF list + fix Windows build boundary
uninstall_driver() uses inf_list[0] without checking inf_list (Line 103/105). Also the comment says pnputil flags are available starting build 14393, but the check is > 14393 (Line 102), which excludes 14393 itself.
Proposed fix
inf_list_all = _pnpdrv_info(session, device_name, ["InfName"])
inf_list = list(set(inf_list_all))
+ if not inf_list:
+ test.error(f"Could not find installed INF name for device '{device_name}' ({driver_name})")
@@
- if int(build_ver) > 14393:
+ if int(build_ver) >= 14393:
uninst_store_cmd = "pnputil /delete-driver %s /uninstall /force" % inf_list[0]🤖 Prompt for AI Agents
In @provider/windows/win_driver_utils.py around lines 77 - 120, The
uninstall_driver function currently uses inf_list[0] without checking for an
empty list and uses a strict > 14393 check that excludes build 14393; update
uninstall_driver to compute inf_list = list(set(_pnpdrv_info(session,
device_name, ["InfName"]))) then if not inf_list call test.error (or return)
with a clear message that no INF was found (include
device_name/device_hwid/output context) to avoid an IndexError, and change the
pnputil condition from if int(build_ver) > 14393 to if int(build_ver) >= 14393
so build 14393 uses the newer flags; reference symbols: uninstall_driver,
_pnpdrv_info, inf_list, build_ver, pnputil.
| def run_installer(vm, session, test, params, run_installer_cmd): | ||
| """ | ||
| Install/uninstall/repair virtio-win drivers and qxl,spice and | ||
| qemu-ga-win by installer. | ||
| If installer(virtio-win) version is in [1.9.37, 1.9.40] | ||
| then installer itself will restart vm for installation and | ||
| uninstallation function; otherwise there is no need to reboot guest. | ||
| While for repair function, installer itself always restart vm by | ||
| itself; | ||
| :param vm: vm object. | ||
| :param session: The guest session object. | ||
| :param test: kvm test object | ||
| :param params: the dict used for parameters | ||
| :param run_installer_cmd: install/uninstall/repair cmd. | ||
| :return session: a new session after restart of installer | ||
| """ | ||
| cdrom_virtio = params["cdrom_virtio"] | ||
| installer_restart_version = params.get( | ||
| "installer_restart_version", "[1.9.37.0, 1.9.40.0]" | ||
| ) | ||
| cdrom_virtio_path = os.path.basename( | ||
| utils_misc.get_path(data_dir.get_data_dir(), cdrom_virtio) | ||
| ) | ||
| match = re.search(r"virtio-win-(\d+\.\d+(?:\.\d+)?-\d+)", cdrom_virtio_path) | ||
| cdrom_virtio_version = re.sub("-", ".", match.group(1)) | ||
| # run installer cmd | ||
| run_installer_cmd = utils_misc.set_winutils_letter(session, run_installer_cmd) | ||
| session.cmd(run_installer_cmd) | ||
|
|
||
| if not utils_misc.wait_for( | ||
| lambda: not autoit_installer_check(params, session), 240, 2, 2 | ||
| ): | ||
| test.fail("Autoit exe stop there for 240s, please have a check.") | ||
| restart_con_ver = cdrom_virtio_version in VersionInterval(installer_restart_version) | ||
| restart_con_repair = "repair" in run_installer_cmd | ||
| if restart_con_ver or restart_con_repair: | ||
| # Wait for vm re-start by installer itself | ||
| if not utils_misc.wait_for(lambda: not session.is_responsive(), 120, 5, 5): | ||
| test.fail( | ||
| "The previous session still exists,seems that the vm doesn't restart." | ||
| ) | ||
| session = vm.wait_for_login(timeout=360) | ||
| # for the early virtio-win instller, rebooting is needed. | ||
| if cdrom_virtio_version in VersionInterval("(,1.9.37.0)"): | ||
| session = vm.reboot(session) | ||
| return session | ||
|
|
There was a problem hiding this comment.
Guard ISO version regex match and add timeout for installer command
run_installer() assumes re.search(...) matches and immediately uses match.group(1) (Lines 242-244). If the ISO naming differs, this crashes with AttributeError. Also session.cmd(run_installer_cmd) has no timeout (Line 246).
Proposed fix
match = re.search(r"virtio-win-(\d+\.\d+(?:\.\d+)?-\d+)", cdrom_virtio_path)
- cdrom_virtio_version = re.sub("-", ".", match.group(1))
+ if not match:
+ test.error(f"Could not parse virtio-win version from '{cdrom_virtio_path}'")
+ cdrom_virtio_version = re.sub("-", ".", match.group(1))
@@
- session.cmd(run_installer_cmd)
+ session.cmd(run_installer_cmd, timeout=INSTALL_TIMEOUT)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @provider/windows/win_driver_utils.py around lines 218 - 265, In
run_installer(), guard the regex result before using match.group(1): check if
match is None and either fail the test with a clear message (e.g.,
test.fail("Unexpected virtio ISO name: {cdrom_virtio_path}")) or derive a safe
default for cdrom_virtio_version; then continue. Also add a timeout to the
installer invocation by calling session.cmd(run_installer_cmd,
timeout=<reasonable_seconds>) instead of session.cmd(run_installer_cmd) so the
call doesn't block forever. Update references: the match variable from
re.search(...), cdrom_virtio_version assignment, and the
session.cmd(run_installer_cmd) call.
| def load_driver(session, test, params, load_method="enable"): | ||
| """ | ||
| Load driver. | ||
| :param session: The guest session object | ||
| :param test: kvm test object | ||
| :param params: the dict used for parameters | ||
| :param load_method: Load driver method | ||
| """ | ||
| driver_name = params["driver_name"] | ||
| devcon_path = params["devcon_path"] | ||
| if load_method != "enable": | ||
| params.get("virtio_win_media_type", "iso") | ||
|
|
||
| device_hwid = driver_info_dict[driver_name]["hwid"] | ||
| install_driver_by_virtio_media(session, test, devcon_path, device_hwid) | ||
| else: | ||
| device_id = get_device_id(session, test, driver_name) | ||
| cmd = '%s enable "@%s"' % (devcon_path, device_id) | ||
| enable_driver(session, test, cmd) | ||
| utils_test.qemu.windrv_verify_running(session, test, driver_name) | ||
|
|
There was a problem hiding this comment.
Fix broken call to install_driver_by_virtio_media() in load_driver()
install_driver_by_virtio_media() takes (session, test, devcon_path, media_type, driver_name, device_hwid) (Lines 158-171), but load_driver() calls it as install_driver_by_virtio_media(session, test, devcon_path, device_hwid) (Line 404). That will raise TypeError immediately.
Proposed fix
def load_driver(session, test, params, load_method="enable"):
@@
driver_name = params["driver_name"]
devcon_path = params["devcon_path"]
if load_method != "enable":
- params.get("virtio_win_media_type", "iso")
-
+ media_type = params.get("virtio_win_media_type", "iso")
device_hwid = driver_info_dict[driver_name]["hwid"]
- install_driver_by_virtio_media(session, test, devcon_path, device_hwid)
+ install_driver_by_virtio_media(
+ session, test, devcon_path, media_type, driver_name, device_hwid
+ )Also applies to: 158-195
🤖 Prompt for AI Agents
In @provider/windows/win_driver_utils.py around lines 389 - 410, The call to
install_driver_by_virtio_media in load_driver is missing arguments and will
raise a TypeError; retrieve the virtio media type into a variable (virtio_media
= params.get("virtio_win_media_type", "iso")), then call
install_driver_by_virtio_media(session, test, devcon_path, virtio_media,
driver_name, device_hwid) so the function receives (session, test, devcon_path,
media_type, driver_name, device_hwid) as required; update the non-enable branch
of load_driver accordingly.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @libvirt/tests/src/virtual_network/qemu/netperf.py:
- Around line 256-261: The comparison uses the aggregated variable driver_name
instead of the loop variable drv_name so the condition driver_name == "VIOPROT"
never matches; change the conditional to check the loop variable (drv_name ==
"VIOPROT") where the install block begins (the if block that logs via
test.log.info and builds install_cmd from params.get("install_vioprot_cmd") and
calls session.cmd_status_output), leaving the rest of the install flow
(test.error on non-zero status) unchanged.
In @provider/virtual_network/netperf_base.py:
- Around line 88-100: The netperf_thread function computes n as
abs(int(params.get("numa_node"))) - 1 which can yield -1 for "0" or raise
TypeError for None; validate params.get("numa_node") first (ensure it's not None
and is a non-negative integer string) and compute n only if the parsed value >=
1, otherwise skip adding the numactl prefix or raise a clear ValueError; update
the logic around the n calculation and the numactl invocation in netperf_thread
so you only call "numactl --cpunodebind=%s --membind=%s" with a verified
non-negative n and log or error when the input is invalid.
- Around line 1-8: Remove the unused Python 2 compatibility import and use
native str types: delete the import of six and replace any isinstance(...,
six.string_types) usage (notably in the function/variable that checks result at
the spot using six.string_types) with isinstance(result, str); ensure no other
references to six remain in netperf_base.py.
In @provider/windows/win_driver_utils.py:
- Around line 242-243: The code assumes the regex match on cdrom_virtio_path
always succeeds and calls match.group(1) unguarded; add a check for match before
using group(1) (e.g., if match: cdrom_virtio_version = re.sub("-", ".",
match.group(1)) else: handle missing match by setting cdrom_virtio_version to
None or raising a clear ValueError/logging an error with cdrom_virtio_path) so
you avoid AttributeError when the pattern doesn't match.
- Around line 400-404: The code currently ignores the result of
params.get("virtio_win_media_type", "iso") and calls
install_driver_by_virtio_media with too few arguments; fix by assigning that
params.get result to a variable (e.g., media_type) and call
install_driver_by_virtio_media(session, test, devcon_path, media_type,
driver_name, device_hwid) so the call matches the function signature and uses
the previously retrieved device_hwid from driver_info_dict[driver_name]["hwid"].
- Around line 375-378: The code accesses output[0] from _pnpdrv_info(session,
device_name, ["DeviceID"]) without bounds checking which can cause IndexError;
update the block around _pnpdrv_info, output, and device_id to check if output
is truthy (e.g., if not output: handle missing device by returning None or
raising a clear error and/or logging) before assigning device_id = output[0],
and then proceed to unescape "&" only when device_id is present.
🧹 Nitpick comments (11)
provider/windows/win_driver_utils.py (3)
24-63: Consider consistent use of raw strings for all HWID entries.Line 60 uses a raw string (
r"...") for the HWID, while other entries (lines 26, 30, 34, etc.) don't, despite all containing backslashes in PCI device identifiers. For consistency and clarity, consider using raw strings for all HWID values.♻️ Proposed refactor for consistency
Apply raw string prefix to all hwid values:
driver_info_dict = { "netkvm": { - "hwid": '"PCI\\VEN_1AF4&DEV_1000" "PCI\\VEN_1AF4&DEV_1041"', + "hwid": r'"PCI\VEN_1AF4&DEV_1000" "PCI\VEN_1AF4&DEV_1041"', "device_name": "Red Hat VirtIO Ethernet Adapter", }, "viorng": { - "hwid": '"PCI\\VEN_1AF4&DEV_1005" "PCI\\VEN_1AF4&DEV_1044"', + "hwid": r'"PCI\VEN_1AF4&DEV_1005" "PCI\VEN_1AF4&DEV_1044"', "device_name": "VirtIO RNG Device", }, # ... apply to remaining entries
158-195: Simplify boolean assignment on line 192.Line 192 uses
installed_any |= True, which is valid but unnecessarily complex. SinceTrue | anythingis alwaysTrue, this can be simplified toinstalled_any = True.♻️ Proposed refactor
- installed_any |= True + installed_any = True
284-284: Consider replacing hard-coded sleep with a wait mechanism.The hard-coded 15-second sleep could be replaced with
utils_misc.wait_forto poll for VM responsiveness, making the code more robust and potentially faster.♻️ Proposed refactor
- time.sleep(15) - return vm.wait_for_login(timeout=360) + # Wait for VM to become unresponsive (reboot initiated) + if not utils_misc.wait_for(lambda: not session.is_responsive(), 30, 1, 1): + LOG_JOB.warning("VM did not reboot as expected after MSI uninstall") + return vm.wait_for_login(timeout=360)provider/virtual_network/network_base.py (3)
658-675: LGTM! Minor formatting issue on log statement.The
pin_vcpusfunction is well-structured. However, the log statement has inconsistent indentation.🔧 Optional fix for indentation
for vcpu in range(vcpu_count): cpu = cpus[vcpu] vm.vcpupin(vcpu, str(cpu)) - LOG.info("Pinned %d vcpu of vm %s to host cpu %s", - vcpu, vm.name, cpu) + LOG.info("Pinned %d vcpu of vm %s to host cpu %s", + vcpu, vm.name, cpu) return vcpu_count
705-719: Consider addingstrict=Truetozip()for safety.If
vhost_tidsandcpushave different lengths,zip()silently drops extra items. While the caller (pin_vcpu_vhost_threads) currently ensures the lists are compatible, addingstrict=Truewould catch any future misuse.🔧 Suggested fix
- for tid, cpu in zip(vhost_tids, cpus): + for tid, cpu in zip(vhost_tids, cpus, strict=True):
751-752: Chain the exception for better debugging context.When re-raising as
TestError, useraise ... from eto preserve the original traceback.🔧 Suggested fix
except Exception as e: - raise exceptions.TestError("Failed to pin VM threads: %s" % e) + raise exceptions.TestError("Failed to pin VM threads: %s" % e) from eprovider/virtual_network/vdpa_utils.py (1)
66-81: Remove unusedovsparameter and fix docstring typo.The
ovsparameter is declared but never used. Also, there's a typo in the docstring: "parma" should be "param".🔧 Suggested fix
-def add_flows_to_ovs_bridge(bridge, ovs): +def add_flows_to_ovs_bridge(bridge): """ Add flow rules to the given ovs bridge. - :parma bridge: OVS bridge name + :param bridge: OVS bridge name :type bridge: str - :param ovs: OVS instance - :type ovs: OpenVSwitch """Also update the caller at line 125:
- add_flows_to_ovs_bridge(ovs_br, self.ovs) + add_flows_to_ovs_bridge(ovs_br)libvirt/tests/src/virtual_network/qemu/netperf.py (4)
134-136: Consider usingast.literal_evalfor safer parsing.While
eval()on config params is a common pattern in tp-libvirt,ast.literal_eval()would be safer for parsing dict/list literals.🔧 Suggested fix
+import ast + - vm_attrs = eval(params.get('vm_attrs', '{}')) + vm_attrs = ast.literal_eval(params.get('vm_attrs', '{}'))
603-614: Improve parameter naming and remove unused parameters.
- Variable
lis ambiguous (PEP8 E741) - consider renaming totest_durationorduration.- Parameter
portis declared but never used.🔧 Suggested fix
def launch_client( sessions, server, server_ctl, host, clients, - l, + duration, nf_args, - port, params, test, ):Update the caller in
start_testaccordingly and replacelwithdurationthroughout the function.
745-750: Avoid bareexcept- catch specific exceptions or at minimum log the error.The bare
exceptclause silently swallows all exceptions. Consider catching a specific exception or at least logging what was caught.🔧 Suggested fix
def all_clients_up(): try: content = netperf_base.ssh_cmd(clients[-1], "cat %s" % fname) - except: + except Exception as e: content = "" + LOG_JOB.debug("Failed to read netperf file: %s", e) return False
856-858: Remove unreachablebreakstatement.The
breakafterreturn retis unreachable dead code.🔧 Suggested fix
ret["thu"] = parse_demo_result(fname, int(sessions)) return ret - break
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.pyprovider/virtual_network/vdpa_utils.pyprovider/windows/win_driver_utils.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6754
File: libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_operation.cfg:9-10
Timestamp: 2026-01-07T16:04:00.667Z
Learning: In tp-libvirt test configurations, parameters like `master_images_clone` and `remove_image_image1` are processed by the external avocado-vt/virttest framework's image preprocessing logic, not by test files within the tp-libvirt repository itself.
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.py
📚 Learning: 2026-01-07T16:04:00.667Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6754
File: libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_operation.cfg:9-10
Timestamp: 2026-01-07T16:04:00.667Z
Learning: In tp-libvirt test configurations, parameters like `master_images_clone` and `remove_image_image1` are processed by the external avocado-vt/virttest framework's image preprocessing logic, not by test files within the tp-libvirt repository itself.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/netperf.py
📚 Learning: 2025-12-12T10:00:16.768Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:16.768Z
Learning: In tp-libvirt test cfg files, the cartesian config parser supports variable substitution using `${variable_name}` syntax. Variables defined in the cfg file (e.g., `filesize = 4000`) are automatically substituted into strings that reference them (e.g., `count=${filesize}`) before the Python test code receives the parameters.
Applied to files:
libvirt/tests/src/virtual_network/qemu/netperf.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/netperf.py (3)
provider/virtual_network/vdpa_utils.py (2)
OVSHandler(84-132)get_vdpa_ovs_info(92-132)provider/windows/win_driver_utils.py (2)
get_driver_inf_path(123-155)uninstall_driver(77-120)provider/virtual_network/network_base.py (1)
pin_vcpu_vhost_threads(722-752)
🪛 Ruff (0.14.10)
provider/virtual_network/network_base.py
699-699: Function call with shell=True parameter identified, security issue
(S604)
713-713: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
715-715: Function call with shell=True parameter identified, security issue
(S604)
751-751: Do not catch blind exception: Exception
(BLE001)
752-752: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
provider/virtual_network/netperf_base.py
43-43: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
47-47: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
80-80: Function call with shell=True parameter identified, security issue
(S604)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
provider/virtual_network/vdpa_utils.py
16-16: Function call with shell=True parameter identified, security issue
(S604)
33-33: Function call with shell=True parameter identified, security issue
(S604)
49-49: Function call with shell=True parameter identified, security issue
(S604)
63-63: Function call with shell=True parameter identified, security issue
(S604)
66-66: Unused function argument: ovs
(ARG001)
libvirt/tests/src/virtual_network/qemu/netperf.py
103-103: Function call with shell=True parameter identified, security issue
(S604)
134-134: Use of possibly insecure function; consider using ast.literal_eval
(S307)
152-152: Use of possibly insecure function; consider using ast.literal_eval
(S307)
307-307: Function call with shell=True parameter identified, security issue
(S604)
312-312: Function call with shell=True parameter identified, security issue
(S604)
338-338: Loop control variable i not used within loop body
(B007)
403-403: Function call with shell=True parameter identified, security issue
(S604)
410-410: Probable insecure usage of temporary file or directory: "/tmp/log_guestinfo.sh"
(S108)
585-585: Function call with shell=True parameter identified, security issue
(S604)
609-609: Ambiguous variable name: l
(E741)
611-611: Unused function argument: port
(ARG001)
618-618: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netperf"
(S108)
619-619: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netserver"
(S108)
628-628: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
668-668: Avoid specifying long messages outside the exception class
(TRY003)
729-729: Unused function argument: timeout
(ARG001)
730-730: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
732-732: Probable insecure usage of temporary file or directory: "/tmp/netperf_agent.py %d %s -D 1 -H %s -l %s %s"
(S108)
745-745: Do not use bare except
(E722)
797-797: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
🔇 Additional comments (18)
provider/windows/win_driver_utils.py (10)
1-23: LGTM!Imports and timeout constants are well-defined and appropriate for driver installation operations.
66-74: LGTM!The WMI query helper is correctly implemented.
77-121: LGTM!The function correctly handles Windows version differences in pnputil flags and appropriately handles different exit codes for pnputil (0, 3010) vs devcon (0, 1).
123-156: LGTM!The dynamic attribute lookup pattern with proper error handling is appropriate for supporting multiple virtio-win media types.
197-216: LGTM!Exception handling correctly deals with VM reboot scenarios during AutoIt installer execution.
288-329: LGTM!File copying logic with error handling is correctly implemented.
331-343: LGTM!Driver enable logic is straightforward and correct.
345-362: LGTM!The function correctly handles reboot scenarios and returns the session in all non-failure cases.
412-435: LGTM!The unload_driver function correctly handles both enable/disable and uninstall methods with proper parameter passing.
437-458: LGTM!The memory leak check orchestration is correctly implemented with appropriate wait times for driver verifier to detect issues.
provider/virtual_network/network_base.py (1)
678-702: LGTM!The
get_vhost_tidsfunction correctly handles both old (qemu user threads) and new (kernel threads) environments with appropriate fallback logic.provider/virtual_network/vdpa_utils.py (1)
84-131: LGTM!The
OVSHandlerclass correctly handles the case where OVS is not available and provides a clean interface for VDPA/OVS information retrieval.libvirt/tests/cfg/virtual_network/qemu/netperf.cfg (3)
1-89: LGTM!The test configuration is well-structured with appropriate metadata, environment setup commands, and architecture-specific handling. The hardcoded credentials are acceptable defaults for test environments.
113-127: LGTM!The protocol/MTU variant structure is clear: jumbo frames (MTU 9000) for throughput tests (TCP_STREAM, TCP_MAERTS) and standard MTU (1500) for latency tests (TCP_RR).
128-166: LGTM!The two main test variants (
bridge_virtinstallandvhost_vdpa_xml) provide flexibility for different VM creation modes. Based on learnings, themaster_images_cloneandremove_image_image1parameters are processed by the avocado-vt framework.libvirt/tests/src/virtual_network/qemu/netperf.py (1)
46-165: Well-structured VM setup with both XML and virt-install modes.The
runfunction properly handles bothuse_vmxml=yes(XML modification) anduse_vmxml=no(virt-install) modes, with appropriate backup/restore logic for VM cleanup.provider/virtual_network/netperf_base.py (2)
103-119: LGTM!The
format_resultfunction correctly handles string, int, and float types with appropriate format specifiers.
122-147: LGTM!The
netperf_recordfunction provides clean result formatting with configurable column widths and decimal precision.
| if driver_name == "VIOPROT": | ||
| test.log.info("Will install inf file found at '%s'", inf_path) | ||
| install_cmd = params.get("install_vioprot_cmd") % inf_path | ||
| status, output = session.cmd_status_output(install_cmd) | ||
| if status: | ||
| test.error("Install inf file failed, output=%s" % output) |
There was a problem hiding this comment.
Potential logic bug: driver name comparison will never match.
At line 215, driver_name is set to "netkvm VIOPROT" (a space-separated string). The comparison driver_name == "VIOPROT" at line 256 will never be true because driver_name contains both driver names.
This appears to be intended to check if the current driver in the loop (drv_name) is "VIOPROT", not driver_name.
🐛 Suggested fix
- if driver_name == "VIOPROT":
+ if drv_name == "VIOPROT":📝 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 driver_name == "VIOPROT": | |
| test.log.info("Will install inf file found at '%s'", inf_path) | |
| install_cmd = params.get("install_vioprot_cmd") % inf_path | |
| status, output = session.cmd_status_output(install_cmd) | |
| if status: | |
| test.error("Install inf file failed, output=%s" % output) | |
| if drv_name == "VIOPROT": | |
| test.log.info("Will install inf file found at '%s'", inf_path) | |
| install_cmd = params.get("install_vioprot_cmd") % inf_path | |
| status, output = session.cmd_status_output(install_cmd) | |
| if status: | |
| test.error("Install inf file failed, output=%s" % output) |
🤖 Prompt for AI Agents
In @libvirt/tests/src/virtual_network/qemu/netperf.py around lines 256 - 261,
The comparison uses the aggregated variable driver_name instead of the loop
variable drv_name so the condition driver_name == "VIOPROT" never matches;
change the conditional to check the loop variable (drv_name == "VIOPROT") where
the install block begins (the if block that logs via test.log.info and builds
install_cmd from params.get("install_vioprot_cmd") and calls
session.cmd_status_output), leaving the rest of the install flow (test.error on
non-zero status) unchanged.
| import logging | ||
| import os | ||
|
|
||
| import six | ||
| from avocado.utils import process | ||
| from virttest import data_dir, error_context, remote, utils_misc, utils_test | ||
|
|
||
| LOG_JOB = logging.getLogger("avocado.test") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
avocado-vt Python version support requirements 2024
💡 Result:
Short answer: in 2024 Avocado‑VT required Python 3 (no Python 2). Supported Python versions depended on the Avocado core used, but Avocado had moved to Python‑3-only and, during 2024, commonly supported Python 3.6+ (minimum raised to 3.6 in releases such as 92.0) while later docs/release notes and PyPI/wheel metadata show builds as py3 for newer 2024 plugin releases (e.g. avocado‑vt 103/104). See the Avocado VT getting‑started/install docs and Avocado release notes. [1][2][3]
Sources
- Avocado‑VT Getting Started / Installing (Avocado‑VT docs). [1]
- Avocado 92.0 release notes (notes that minimal Python is 3.6 and Python‑3‑only). [2]
- avocado‑framework‑plugin‑vt releases / wheel metadata (py3 wheels: 103.0/104.0 published in 2024). [3]
🏁 Script executed:
# Check if 'six' is actually used in the netperf_base.py file
rg "six\." provider/virtual_network/netperf_base.py
# Also check the entire file to see all imports and usage
cat -n provider/virtual_network/netperf_base.py | head -50Repository: autotest/tp-libvirt
Length of output: 2197
🏁 Script executed:
# Look for setup.py or requirements files to check Python version constraints
fd -t f "setup.py|setup.cfg|pyproject.toml|requirements.txt" -x grep -l "python_requires\|Programming Language :: Python" {}Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Check all six usages in the entire file
cat provider/virtual_network/netperf_base.py | grep -n "six"
# Get the complete file length to understand context
wc -l provider/virtual_network/netperf_base.pyRepository: autotest/tp-libvirt
Length of output: 169
🏁 Script executed:
# Search for all six usages across the codebase
rg "six\." --type py -c | head -20
# Check for setup.py or similar files with Python version constraints
find . -maxdepth 2 -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" | head -5Repository: autotest/tp-libvirt
Length of output: 235
🏁 Script executed:
# Check if there are any requirements files
git ls-files | grep -E "(requirements|setup)" | head -10Repository: autotest/tp-libvirt
Length of output: 103
🏁 Script executed:
# Check the requirements files for six
cat requirements.txt | grep -i six
# Also check the requirements-travis.txt
cat requirements-travis.txt | grep -i six
# Look at the context around line 111 where six.string_types is used
sed -n '105,115p' provider/virtual_network/netperf_base.pyRepository: autotest/tp-libvirt
Length of output: 436
🏁 Script executed:
# Check if this codebase has a setup.py or pyproject.toml that specifies Python version
cat setup.py 2>/dev/null || cat pyproject.toml 2>/dev/null || echo "No setup files found at root"
# Check for any Python version constraints in the repo
rg "python_requires|Programming Language :: Python" --type toml --type py | head -10Repository: autotest/tp-libvirt
Length of output: 91
🏁 Script executed:
# Let me check the actual requirements files to see if six is listed as a dependency
cat requirements.txt
cat requirements-travis.txtRepository: autotest/tp-libvirt
Length of output: 577
🏁 Script executed:
# Also check what six.string_types is equivalent to in Python 3
# and verify there are no other six usages we might have missed
grep -n "six" provider/virtual_network/netperf_base.pyRepository: autotest/tp-libvirt
Length of output: 124
Remove the six import on line 4—it's not required for Python 3-only code.
The avocado-vt framework supports Python 3 only (3.6+). Replace six.string_types on line 111 with the native Python 3 equivalent str:
if isinstance(result, str): # instead of isinstance(result, six.string_types)🤖 Prompt for AI Agents
In @provider/virtual_network/netperf_base.py around lines 1 - 8, Remove the
unused Python 2 compatibility import and use native str types: delete the import
of six and replace any isinstance(..., six.string_types) usage (notably in the
function/variable that checks result at the spot using six.string_types) with
isinstance(result, str); ensure no other references to six remain in
netperf_base.py.
| def netperf_thread(params, numa_enable, client_s, option, fname): | ||
| """ | ||
| Start netperf thread on client | ||
| """ | ||
| cmd = "" | ||
| if numa_enable: | ||
| n = abs(int(params.get("numa_node"))) - 1 | ||
| cmd += "numactl --cpunodebind=%s --membind=%s " % (n, n) | ||
| cmd += option | ||
| cmd += " >> %s" % fname | ||
| LOG_JOB.info("Start netperf thread by cmd '%s'", cmd) | ||
| ssh_cmd(client_s, cmd) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's search for numa_node usage across the codebase
rg -n 'numa_node' --type py --type cfg -A 2 -B 2Repository: autotest/tp-libvirt
Length of output: 90
🏁 Script executed:
# Also search for how this parameter is typically set in config files
find . -name "*.cfg" -o -name "*.conf" | head -20Repository: autotest/tp-libvirt
Length of output: 960
🏁 Script executed:
# Look for validation of numa_node specifically
rg -n 'numa_node.*=' --type py -A 3Repository: autotest/tp-libvirt
Length of output: 21333
🏁 Script executed:
# Find the netperf_base.py file and examine it
find . -name "netperf_base.py" -type fRepository: autotest/tp-libvirt
Length of output: 105
🏁 Script executed:
# Search for netperf-related config files
find . -path "*netperf*" -type f | grep -E "\.(cfg|conf)$" | head -20Repository: autotest/tp-libvirt
Length of output: 443
🏁 Script executed:
# Look for where netperf_thread is called and how numa_node is passed
rg -n 'netperf_thread' --type py -B 3 -A 3Repository: autotest/tp-libvirt
Length of output: 1064
🏁 Script executed:
# Read the full netperf_base.py file
cat -n ./provider/virtual_network/netperf_base.pyRepository: autotest/tp-libvirt
Length of output: 5954
🏁 Script executed:
# Check one of the netperf config files to see how numa_node is set
cat ./libvirt/tests/cfg/virtual_network/qemu/netperf.cfg | grep -A 5 -B 5 "numa"Repository: autotest/tp-libvirt
Length of output: 541
🏁 Script executed:
# Check the main netperf test that calls netperf_thread to understand the context
sed -n '730,750p' ./libvirt/tests/src/virtual_network/qemu/netperf.pyRepository: autotest/tp-libvirt
Length of output: 760
🏁 Script executed:
# Search for numa_node values in all netperf config files
grep -r "numa_node" ./libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
grep -r "numa_node" ./libvirt/tests/cfg/Repository: autotest/tp-libvirt
Length of output: 10989
🏁 Script executed:
# Check if there's default handling for numa_node in the test code
rg -n 'numa_node' ./libvirt/tests/src/virtual_network/qemu/netperf.py -B 3 -A 3Repository: autotest/tp-libvirt
Length of output: 358
🌐 Web query:
numactl --cpunodebind negative number invalid
💡 Result:
That error means numactl was given a node or CPU identifier that the current environment/kernel/numactl library considers out of range or invalid (often because the node mask or cpu list available to the process differs from the system-wide view). Common causes and fixes:
-
You’re running inside a constrained cpuset (cgroups) that restricts visible memory nodes; numactl’s requested node(s) aren’t available there. Check the cpuset mems and adjust (echo appropriate node list into /dev/cpuset/.../mems or use the root cpuset). [1][3]
-
The node exists but has 0 MB memory (empty NUMA node); some numactl/libnuma operations reject binding to nodes with no memory. Verify node sizes with numactl --hardware and /sys/devices/system/node. [1]
-
A libnuma/numactl bug or mismatched versions can mis-parse large or unusual CPU/node numbering (or produce “out of range”/“invalid” messages). Upgrading numactl/libnuma to a fixed version can resolve it. Check distro bug trackers (Ubuntu/Red Hat) and upstream numactl for fixes. [2][5]
Quick diagnostic commands
- numactl --hardware
- numactl --show
- cat /sys/devices/system/node/online
- cat /sys/fs/cgroup/cpuset//mems (or /dev/cpuset/…/mems)
- ls /sys/devices/system/node/node*/meminfo
If you post the exact command you ran and the outputs of numactl --hardware and the cpuset mems, I’ll give specific next steps.
Sources: distro bug/KB pages and numactl bug reports. [1] [3] [2]
References
[1] Ubuntu/launchpad bug and examples showing empty NUMA nodes and cpuset interaction.
[2] Red Hat knowledge articles about interleave/cpuset and numactl errors.
[3] Red Hat / Ubuntu bug reports and numactl package issues.
🏁 Script executed:
# Check network_base.pin_vcpu_vhost_threads function to see how numa_node is used
rg -n 'pin_vcpu_vhost_threads' provider/virtual_network/ -A 15Repository: autotest/tp-libvirt
Length of output: 1268
🏁 Script executed:
# Look for any validation of numa_node before the netperf_thread call
sed -n '700,750p' ./libvirt/tests/src/virtual_network/qemu/netperf.pyRepository: autotest/tp-libvirt
Length of output: 1821
🏁 Script executed:
# Search for default or expected values for numa_node in netperf context
rg -n 'numa_node' ./libvirt/tests/src/virtual_network/ -B 2 -A 2Repository: autotest/tp-libvirt
Length of output: 585
🏁 Script executed:
# Final check: Look at the exact context where netperf_thread is called to see if numa_node is guaranteed
sed -n '310,330p' ./libvirt/tests/src/virtual_network/qemu/netperf.pyRepository: autotest/tp-libvirt
Length of output: 835
Add validation for NUMA node calculation to prevent invalid node references.
The calculation abs(int(params.get("numa_node"))) - 1 can produce -1 when numa_node="0", which is invalid for numactl --cpunodebind. Additionally, if params.get("numa_node") returns None, int(None) will raise a TypeError. Add validation to ensure the resulting node value is non-negative and valid before passing it to numactl, or add explicit documentation about expected input values (e.g., numa_node must be "1" or higher).
🤖 Prompt for AI Agents
In @provider/virtual_network/netperf_base.py around lines 88 - 100, The
netperf_thread function computes n as abs(int(params.get("numa_node"))) - 1
which can yield -1 for "0" or raise TypeError for None; validate
params.get("numa_node") first (ensure it's not None and is a non-negative
integer string) and compute n only if the parsed value >= 1, otherwise skip
adding the numactl prefix or raise a clear ValueError; update the logic around
the n calculation and the numactl invocation in netperf_thread so you only call
"numactl --cpunodebind=%s --membind=%s" with a verified non-negative n and log
or error when the input is invalid.
| match = re.search(r"virtio-win-(\d+\.\d+(?:\.\d+)?-\d+)", cdrom_virtio_path) | ||
| cdrom_virtio_version = re.sub("-", ".", match.group(1)) |
There was a problem hiding this comment.
Fix potential AttributeError from unguarded regex match.
Line 243 calls match.group(1) without checking if match is None. If the regex on line 242 fails to match cdrom_virtio_path, this will raise AttributeError: 'NoneType' object has no attribute 'group'.
🐛 Proposed fix
match = re.search(r"virtio-win-(\d+\.\d+(?:\.\d+)?-\d+)", cdrom_virtio_path)
+ if not match:
+ test.error(
+ "Could not extract version from cdrom path: %s" % cdrom_virtio_path
+ )
cdrom_virtio_version = re.sub("-", ".", match.group(1))🤖 Prompt for AI Agents
In @provider/windows/win_driver_utils.py around lines 242 - 243, The code
assumes the regex match on cdrom_virtio_path always succeeds and calls
match.group(1) unguarded; add a check for match before using group(1) (e.g., if
match: cdrom_virtio_version = re.sub("-", ".", match.group(1)) else: handle
missing match by setting cdrom_virtio_version to None or raising a clear
ValueError/logging an error with cdrom_virtio_path) so you avoid AttributeError
when the pattern doesn't match.
| output = _pnpdrv_info(session, device_name, ["DeviceID"]) | ||
| # workaround for viostor/vioscsi to get data device id | ||
| device_id = output[0] | ||
| device_id = "&".join(device_id.split("&")) |
There was a problem hiding this comment.
Add bounds checking to prevent IndexError.
Line 377 accesses output[0] without checking if output is empty. If _pnpdrv_info returns an empty list (e.g., device not found), this will raise IndexError: list index out of range.
🐛 Proposed fix
output = _pnpdrv_info(session, device_name, ["DeviceID"])
+ if not output:
+ test.fail("No device found for driver '%s' with device name '%s'" % (driver_name, device_name))
# workaround for viostor/vioscsi to get data device id
device_id = output[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.
| output = _pnpdrv_info(session, device_name, ["DeviceID"]) | |
| # workaround for viostor/vioscsi to get data device id | |
| device_id = output[0] | |
| device_id = "&".join(device_id.split("&")) | |
| output = _pnpdrv_info(session, device_name, ["DeviceID"]) | |
| if not output: | |
| test.fail("No device found for driver '%s' with device name '%s'" % (driver_name, device_name)) | |
| # workaround for viostor/vioscsi to get data device id | |
| device_id = output[0] | |
| device_id = "&".join(device_id.split("&")) |
🤖 Prompt for AI Agents
In @provider/windows/win_driver_utils.py around lines 375 - 378, The code
accesses output[0] from _pnpdrv_info(session, device_name, ["DeviceID"]) without
bounds checking which can cause IndexError; update the block around
_pnpdrv_info, output, and device_id to check if output is truthy (e.g., if not
output: handle missing device by returning None or raising a clear error and/or
logging) before assigning device_id = output[0], and then proceed to unescape
"&" only when device_id is present.
|
JOB LOG : /root/avocado/job-results/job-2026-01-09T08.54-59ac558/job.log JOB LOG : /root/avocado/job-results/job-2026-01-09T09.07-3f54917/job.log |
This commit migrates the netperf perf test cases from tp-qemu to tp-libvirt. Most of the code is copied from tp-qemu with minimal changes. Changes include: 1. netperf.cfg: removed Cygwin/VM-to-VM scenario, added libvirt VM configs. - added two main variants: bridge_virtinstall (virt-install mode) and vhost_vdpa_xml (xml modification mode) - Added protocol/mtu combinations: with_jumbo (TCP_STREAM/TCP_MAERTS with MTU 9000) and standard_mtu (TCP_RR with MTU 1500) 2. netperf.py: removed Cygwin/VM-to-VM scenario and use pin functions from network module. - Simplified use_vmxml branch code and added support for both xml modification and virt-install modes 3. netperf_base.py: copied from tp-qemu, removed old pin methods. 4. network_base.py: added new pin functions. 5. vdpa_utils.py & win_driver_utils.py: copied from tp-qemu without modifications. Signed-off-by: Wenli Quan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @libvirt/tests/src/virtual_network/qemu/netperf.py:
- Around line 553-556: The condition uses the wrong dictionary key name causing
the check to always fail: update the conditional to check for ret.get("tx_pkts")
(the same key produced by get_state()) instead of ret.get("tx_pkt"); locate the
check around the tpkt_per_exit computation that references ret and get_state()
and ensure the key names match exactly so tpkt_per_exit is computed when tx_pkts
and exits are present.
- Around line 857-858: There is an unreachable `break` after a `return ret` (the
`break` following `return ret` can never execute); remove the `break` statement
or, if the intent was to exit a loop before returning, move the `break` to
before the `return` so the control flow is correct (look for the `return
ret`/`ret` usage in the surrounding function to decide which fix to apply).
In @provider/virtual_network/network_base.py:
- Around line 673-674: The LOG.info call spanning two lines has inconsistent
indentation; locate the LOG.info statement that references vcpu, vm.name, and
cpu in network_base.py and reformat it so the continued line is properly aligned
(e.g., keep the format string and arguments on the same line or wrap with
parentheses and align the arguments under the opening parenthesis) to maintain
consistent indentation and comply with project style.
- Around line 670-675: The loop that pins vCPUs uses cpus[vcpu] and can raise
IndexError if vcpu_count > len(cpus); update the loop in the function using
variables vcpu_count, cpus, vm.vcpupin and LOG.info to guard against that by
either validating that vcpu_count <= len(cpus) and raising a clear ValueError,
or by computing cpu = cpus[vcpu % len(cpus)] to wrap around; ensure vm.name
remains used in the LOG.info message and add a brief log or comment when
wrapping/validation occurs to make behavior explicit.
In @provider/virtual_network/vdpa_utils.py:
- Around line 101-103: There's a typo in the docstring where "Ture" is used
instead of "True" in the description of the return behavior (the phrase
"return_port is Ture"); update that string to "return_port is True" in the
docstring (the comment around the return description for the function that
mentions "return_port") so the documentation is correct.
In @provider/windows/win_driver_utils.py:
- Around line 400-404: The call to install_driver_by_virtio_media is passing
only 4 args but the function requires 6; retrieve the media_type from
params.get("virtio_win_media_type", "iso") and pass it plus the driver_name into
the call. Specifically, keep using session, test, devcon_path, device_hwid, then
append media_type (from params.get) and driver_name (from the current
scope/driver_info_dict lookup) so install_driver_by_virtio_media(session, test,
devcon_path, device_hwid, media_type, driver_name) is invoked.
🧹 Nitpick comments (10)
provider/virtual_network/network_base.py (2)
713-719: Addstrict=Falsetozip()for explicit intent.Using
zip()without an explicitstrict=parameter can hide mismatched lengths betweenvhost_tidsandcpus. Addingstrict=Falsedocuments intent.♻️ Suggested change
- for tid, cpu in zip(vhost_tids, cpus): + for tid, cpu in zip(vhost_tids, cpus, strict=False):
751-752: Chain exception for better debugging context.Use
raise ... from eto preserve the original traceback, which aids debugging.♻️ Suggested change
except Exception as e: - raise exceptions.TestError("Failed to pin VM threads: %s" % e) + raise exceptions.TestError("Failed to pin VM threads: %s" % e) from eprovider/virtual_network/vdpa_utils.py (1)
66-81: Unusedovsparameter in function signature.The
ovsparameter is passed but never used in the function body. Either remove it or use it if intended.♻️ Suggested change
-def add_flows_to_ovs_bridge(bridge, ovs): +def add_flows_to_ovs_bridge(bridge): """ Add flow rules to the given ovs bridge. - :parma bridge: OVS bridge name + :param bridge: OVS bridge name :type bridge: str - :param ovs: OVS instance - :type ovs: OpenVSwitch """Also update the call site at line 125:
- add_flows_to_ovs_bridge(ovs_br, self.ovs) + add_flows_to_ovs_bridge(ovs_br)libvirt/tests/src/virtual_network/qemu/netperf.py (6)
745-750: Replace bareexceptwith specific exception types.Bare
exceptclauses catch all exceptions includingSystemExitandKeyboardInterrupt, which can mask critical errors.♻️ Suggested change
def all_clients_up(): try: content = netperf_base.ssh_cmd(clients[-1], "cat %s" % fname) - except: + except Exception: content = "" return False
609-609: Rename ambiguous variablelto a more descriptive name.The variable name
lis easily confused with1(one). Consider renaming totest_durationorduration.
628-628: Remove unused variabletimeout.The variable
timeoutis assigned but never used.♻️ Suggested change
if params.get("os_type") == "windows": - timeout = float(params.get("timeout", "240")) cdrom_drv = utils_misc.get_winutils_vol(server_ctl)
414-416: Use context manager for file handling.The file is opened but closed manually. Using a context manager ensures the file is properly closed even if an exception occurs.
♻️ Suggested change
- logfile = open(path, "a+") - logfile.write(output) - logfile.close() + with open(path, "a+") as logfile: + logfile.write(output)
770-772: Use context manager for file handling inparse_demo_result.♻️ Suggested change
- fd = open(fname) - lines = fd.readlines() - fd.close() + with open(fname) as fd: + lines = fd.readlines()
853-855: Use context manager for file handling.♻️ Suggested change
- f = open(fname, "w") - f.write(finished_result) - f.close() + with open(fname, "w") as f: + f.write(finished_result)provider/windows/win_driver_utils.py (1)
451-454: Usenotinstead ofis Falsefor boolean comparison.Prefer
not vm.is_alive()overvm.is_alive() is Falsefor Pythonic boolean checks.♻️ Suggested change
- if vm.is_alive() is False: + if not vm.is_alive(): test.fail( "VM is not alive after uninstall driver,please check if it is a memory leak" )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qemu/netperf.cfglibvirt/tests/src/virtual_network/qemu/netperf.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.pyprovider/virtual_network/vdpa_utils.pyprovider/windows/win_driver_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/netperf.cfg
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6754
File: libvirt/tests/cfg/virtual_network/qemu/hotplug_nic_with_operation.cfg:9-10
Timestamp: 2026-01-07T16:04:00.667Z
Learning: In tp-libvirt test configurations, parameters like `master_images_clone` and `remove_image_image1` are processed by the external avocado-vt/virttest framework's image preprocessing logic, not by test files within the tp-libvirt repository itself.
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6754
File: libvirt/tests/cfg/virtual_network/qemu/boot_with_multiqueue.cfg:19-21
Timestamp: 2026-01-10T16:53:10.863Z
Learning: In tp-libvirt test configurations for Windows guests, the avocado-vt/virttest framework automatically provides the `cdrom_virtio` parameter for accessing virtio-win ISO, even when not explicitly defined in the cfg file. The Python test code can reference `cdrom_virtio` without requiring it to be set via `cdroms += " virtio"` in the configuration.
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/virtual_network/qemu/netperf.py
📚 Learning: 2025-12-12T10:00:16.768Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:16.768Z
Learning: In tp-libvirt test cfg files, the cartesian config parser supports variable substitution using `${variable_name}` syntax. Variables defined in the cfg file (e.g., `filesize = 4000`) are automatically substituted into strings that reference them (e.g., `count=${filesize}`) before the Python test code receives the parameters.
Applied to files:
libvirt/tests/src/virtual_network/qemu/netperf.py
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/src/virtual_network/qemu/netperf.py
📚 Learning: 2026-01-10T16:53:10.863Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6754
File: libvirt/tests/cfg/virtual_network/qemu/boot_with_multiqueue.cfg:19-21
Timestamp: 2026-01-10T16:53:10.863Z
Learning: In tp-libvirt test configurations for Windows guests, the avocado-vt/virttest framework automatically provides the `cdrom_virtio` parameter for accessing virtio-win ISO, even when not explicitly defined in the cfg file. The Python test code can reference `cdrom_virtio` without requiring it to be set via `cdroms += " virtio"` in the configuration.
Applied to files:
provider/windows/win_driver_utils.py
🧬 Code graph analysis (2)
libvirt/tests/src/virtual_network/qemu/netperf.py (7)
provider/virtual_network/vdpa_utils.py (2)
OVSHandler(84-132)get_vdpa_ovs_info(92-132)libvirt/tests/src/passthrough/robustness/passthrough_robustness.py (1)
start(176-225)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)provider/windows/win_driver_utils.py (2)
get_driver_inf_path(123-155)uninstall_driver(77-120)provider/virtual_network/network_base.py (1)
pin_vcpu_vhost_threads(722-752)libvirt/tests/src/virsh_cmd/network/virsh_net_dhcp_leases.py (1)
f(149-150)
provider/windows/win_driver_utils.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtio_transitional/virtio_transitional_nic.py (1)
reboot(38-44)
🪛 Ruff (0.14.10)
provider/virtual_network/netperf_base.py
43-43: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
47-47: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
80-80: Function call with shell=True parameter identified, security issue
(S604)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
libvirt/tests/src/virtual_network/qemu/netperf.py
103-103: Function call with shell=True parameter identified, security issue
(S604)
134-134: Use of possibly insecure function; consider using ast.literal_eval
(S307)
152-152: Use of possibly insecure function; consider using ast.literal_eval
(S307)
307-307: Function call with shell=True parameter identified, security issue
(S604)
312-312: Function call with shell=True parameter identified, security issue
(S604)
338-338: Loop control variable i not used within loop body
(B007)
403-403: Function call with shell=True parameter identified, security issue
(S604)
410-410: Probable insecure usage of temporary file or directory: "/tmp/log_guestinfo.sh"
(S108)
585-585: Function call with shell=True parameter identified, security issue
(S604)
609-609: Ambiguous variable name: l
(E741)
611-611: Unused function argument: port
(ARG001)
618-618: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netperf"
(S108)
619-619: Probable insecure usage of temporary file or directory: "/tmp/netperf-%s/src/netserver"
(S108)
628-628: Local variable timeout is assigned to but never used
Remove assignment to unused variable timeout
(F841)
668-668: Avoid specifying long messages outside the exception class
(TRY003)
729-729: Unused function argument: timeout
(ARG001)
730-730: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
732-732: Probable insecure usage of temporary file or directory: "/tmp/netperf_agent.py %d %s -D 1 -H %s -l %s %s"
(S108)
745-745: Do not use bare except
(E722)
797-797: Probable insecure usage of temporary file or directory: "/tmp/netperf.%s.nf"
(S108)
provider/virtual_network/vdpa_utils.py
16-16: Function call with shell=True parameter identified, security issue
(S604)
33-33: Function call with shell=True parameter identified, security issue
(S604)
49-49: Function call with shell=True parameter identified, security issue
(S604)
63-63: Function call with shell=True parameter identified, security issue
(S604)
66-66: Unused function argument: ovs
(ARG001)
provider/virtual_network/network_base.py
699-699: Function call with shell=True parameter identified, security issue
(S604)
713-713: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
715-715: Function call with shell=True parameter identified, security issue
(S604)
751-751: Do not catch blind exception: Exception
(BLE001)
752-752: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (7)
provider/virtual_network/vdpa_utils.py (1)
9-81: LGTM for OVS helper functions.The shell-based PCI address resolution functions are appropriate for this test infrastructure context. The pattern of chaining
vdpa dev show,/sys/bus/pci/devices, and/sys/bus/pci/devices/.../net/lookups is reasonable for vdpa/SR-IOV scenarios.provider/virtual_network/netperf_base.py (3)
103-119: LGTM - Result formatting logic is correct.The
format_resultfunction properly handles string, int, and float types with appropriate format specifiers.
122-147: LGTM - Record building logic is correct.The
netperf_recordfunction correctly filters and formats results with optional header generation.
93-100: Handle missing or invalidnuma_nodeparameter in netperf_thread.Line 95 will crash with a TypeError if
params.get("numa_node")returns None (whennuma_enable=Truebutnuma_nodeis not set). Additionally, ifnuma_node="0", the expressionabs(0) - 1produces-1, which is invalid fornumactl --cpunodebind. The code should either validate thatnuma_nodeis provided and >= 1 whennuma_enable=True, or handle the None case gracefully likepin_vcpu_vhost_threads()does at line 318 of network_base.py.libvirt/tests/src/virtual_network/qemu/netperf.py (1)
46-165: LGTM - VM setup and VMXML handling logic.The VM initialization with both XML modification mode and virt-install mode is well-structured. The MAC address handling and interface configuration logic correctly handles the different scenarios.
provider/windows/win_driver_utils.py (2)
66-74: LGTM - WMI query helper function.The
_pnpdrv_infofunction correctly wraps WMI queries for signed driver information.
77-120: LGTM - Driver uninstall logic.The
uninstall_driverfunction properly handles Windows version-specific pnputil flags and acceptable return codes for reboot scenarios.
| if ret.get("tx_pkt") and ret.get("exits"): | ||
| ret["tpkt_per_exit"] = float(ret["tx_pkts"]) / float( | ||
| ret["exits"] | ||
| ) |
There was a problem hiding this comment.
Typo in dictionary key check: tx_pkt should be tx_pkts.
The condition checks for ret.get("tx_pkt") but the key set in get_state() is "tx_pkts" (with 's'). This condition will always be False.
🐛 Proposed fix
- if ret.get("tx_pkt") and ret.get("exits"):
+ if ret.get("tx_pkts") and ret.get("exits"):📝 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 ret.get("tx_pkt") and ret.get("exits"): | |
| ret["tpkt_per_exit"] = float(ret["tx_pkts"]) / float( | |
| ret["exits"] | |
| ) | |
| if ret.get("tx_pkts") and ret.get("exits"): | |
| ret["tpkt_per_exit"] = float(ret["tx_pkts"]) / float( | |
| ret["exits"] | |
| ) |
🤖 Prompt for AI Agents
In @libvirt/tests/src/virtual_network/qemu/netperf.py around lines 553 - 556,
The condition uses the wrong dictionary key name causing the check to always
fail: update the conditional to check for ret.get("tx_pkts") (the same key
produced by get_state()) instead of ret.get("tx_pkt"); locate the check around
the tpkt_per_exit computation that references ret and get_state() and ensure the
key names match exactly so tpkt_per_exit is computed when tx_pkts and exits are
present.
| return ret | ||
| break |
There was a problem hiding this comment.
Unreachable code after return statement.
The break statement on line 858 is never executed because it follows a return statement.
🔧 Proposed fix
ret["thu"] = parse_demo_result(fname, int(sessions))
return ret
- break📝 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.
| return ret | |
| break | |
| ret["thu"] = parse_demo_result(fname, int(sessions)) | |
| return ret |
🤖 Prompt for AI Agents
In @libvirt/tests/src/virtual_network/qemu/netperf.py around lines 857 - 858,
There is an unreachable `break` after a `return ret` (the `break` following
`return ret` can never execute); remove the `break` statement or, if the intent
was to exit a loop before returning, move the `break` to before the `return` so
the control flow is correct (look for the `return ret`/`ret` usage in the
surrounding function to decide which fix to apply).
| for vcpu in range(vcpu_count): | ||
| cpu = cpus[vcpu] | ||
| vm.vcpupin(vcpu, str(cpu)) | ||
| LOG.info("Pinned %d vcpu of vm %s to host cpu %s", | ||
| vcpu, vm.name, cpu) | ||
| return vcpu_count |
There was a problem hiding this comment.
Potential IndexError when vCPU count exceeds available CPUs.
If the VM has more vCPUs than the number of CPUs in the provided list, cpus[vcpu] will raise an IndexError. Consider adding a check or using modulo to wrap around.
🐛 Proposed fix
def pin_vcpus(vm, cpus):
"""
Pin all vcpus of a VM to the given CPU list.
:param vm: libvirt vm object
:param cpus: sorted list of host cpu ids in the node node
:return: number of vcpus pinned
"""
dominfo = vm.dominfo()
vcpu_count = int(dominfo.get("CPU(s)", 0))
+ if vcpu_count > len(cpus):
+ LOG.warning("vCPU count (%d) exceeds available CPUs (%d), will wrap around",
+ vcpu_count, len(cpus))
+
for vcpu in range(vcpu_count):
- cpu = cpus[vcpu]
+ cpu = cpus[vcpu % len(cpus)]
vm.vcpupin(vcpu, str(cpu))
LOG.info("Pinned %d vcpu of vm %s to host cpu %s",
vcpu, vm.name, cpu)
return vcpu_count🤖 Prompt for AI Agents
In @provider/virtual_network/network_base.py around lines 670 - 675, The loop
that pins vCPUs uses cpus[vcpu] and can raise IndexError if vcpu_count >
len(cpus); update the loop in the function using variables vcpu_count, cpus,
vm.vcpupin and LOG.info to guard against that by either validating that
vcpu_count <= len(cpus) and raising a clear ValueError, or by computing cpu =
cpus[vcpu % len(cpus)] to wrap around; ensure vm.name remains used in the
LOG.info message and add a brief log or comment when wrapping/validation occurs
to make behavior explicit.
| LOG.info("Pinned %d vcpu of vm %s to host cpu %s", | ||
| vcpu, vm.name, cpu) |
There was a problem hiding this comment.
Inconsistent indentation in LOG.info call.
The second line of the log statement appears to have incorrect indentation.
🔧 Suggested fix
LOG.info("Pinned %d vcpu of vm %s to host cpu %s",
- vcpu, vm.name, cpu)
+ vcpu, vm.name, cpu)📝 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.
| LOG.info("Pinned %d vcpu of vm %s to host cpu %s", | |
| vcpu, vm.name, cpu) | |
| LOG.info("Pinned %d vcpu of vm %s to host cpu %s", | |
| vcpu, vm.name, cpu) |
🤖 Prompt for AI Agents
In @provider/virtual_network/network_base.py around lines 673 - 674, The
LOG.info call spanning two lines has inconsistent indentation; locate the
LOG.info statement that references vcpu, vm.name, and cpu in network_base.py and
reformat it so the continued line is properly aligned (e.g., keep the format
string and arguments on the same line or wrap with parentheses and align the
arguments under the opening parenthesis) to maintain consistent indentation and
comply with project style.
| :return: list of target interfaces(bridges and ports) if return_port is Ture, | ||
| else empty list | ||
| :rtype: list |
There was a problem hiding this comment.
Typo in docstring: "Ture" should be "True".
📝 Fix typo
- :return: list of target interfaces(bridges and ports) if return_port is Ture,
+ :return: list of target interfaces(bridges and ports) if return_port is True,📝 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.
| :return: list of target interfaces(bridges and ports) if return_port is Ture, | |
| else empty list | |
| :rtype: list | |
| :return: list of target interfaces(bridges and ports) if return_port is True, | |
| else empty list | |
| :rtype: list |
🤖 Prompt for AI Agents
In @provider/virtual_network/vdpa_utils.py around lines 101 - 103, There's a
typo in the docstring where "Ture" is used instead of "True" in the description
of the return behavior (the phrase "return_port is Ture"); update that string to
"return_port is True" in the docstring (the comment around the return
description for the function that mentions "return_port") so the documentation
is correct.
| if load_method != "enable": | ||
| params.get("virtio_win_media_type", "iso") | ||
|
|
||
| device_hwid = driver_info_dict[driver_name]["hwid"] | ||
| install_driver_by_virtio_media(session, test, devcon_path, device_hwid) |
There was a problem hiding this comment.
Incorrect number of arguments passed to install_driver_by_virtio_media.
The function install_driver_by_virtio_media requires 6 arguments, but only 4 are provided here. Missing media_type and driver_name.
🐛 Proposed fix
if load_method != "enable":
- params.get("virtio_win_media_type", "iso")
-
+ media_type = params.get("virtio_win_media_type", "iso")
device_hwid = driver_info_dict[driver_name]["hwid"]
- install_driver_by_virtio_media(session, test, devcon_path, device_hwid)
+ install_driver_by_virtio_media(session, test, devcon_path, media_type, driver_name, device_hwid)📝 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 load_method != "enable": | |
| params.get("virtio_win_media_type", "iso") | |
| device_hwid = driver_info_dict[driver_name]["hwid"] | |
| install_driver_by_virtio_media(session, test, devcon_path, device_hwid) | |
| if load_method != "enable": | |
| media_type = params.get("virtio_win_media_type", "iso") | |
| device_hwid = driver_info_dict[driver_name]["hwid"] | |
| install_driver_by_virtio_media(session, test, devcon_path, media_type, driver_name, device_hwid) |
🤖 Prompt for AI Agents
In @provider/windows/win_driver_utils.py around lines 400 - 404, The call to
install_driver_by_virtio_media is passing only 4 args but the function requires
6; retrieve the media_type from params.get("virtio_win_media_type", "iso") and
pass it plus the driver_name into the call. Specifically, keep using session,
test, devcon_path, device_hwid, then append media_type (from params.get) and
driver_name (from the current scope/driver_info_dict lookup) so
install_driver_by_virtio_media(session, test, devcon_path, device_hwid,
media_type, driver_name) is invoked.
2 participants
This commit migrates the netperf perf test cases from tp-qemu to tp-libvirt. Most of the code is copied from tp-qemu with minimal changes.
Changes include:
(xml modification mode)
and standard_mtu (TCP_RR with MTU 1500)
virt-install modes
ID: LIBVIRTAT-22217, LIBVIRTAT-22242
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.