Migrate netperf perf tests from tp-qemu to tp-libvirt #6743
Conversation
WalkthroughAdds a netperf test suite for libvirt virtual networking comprising: a new test config (libvirt/tests/cfg/virtual_network/qemu/netperf.cfg), a primary test module (libvirt/tests/src/virtual_network/qemu/netperf.py) implementing run/start_test/launch_client flows, provider utilities for netperf orchestration and result formatting (provider/virtual_network/netperf_base.py), NUMA/vCPU and vhost pinning helpers (provider/virtual_network/network_base.py), OVS/vdpa utilities (provider/virtual_network/vdpa_utils.py), and Windows VirtIO driver management utilities (provider/windows/win_driver_utils.py). Changes introduce VM/host/client setup, MTU/NUMA handling, netperf lifecycle, OVS interactions, driver install/uninstall flows, and result recording APIs. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
| def all_clients_up(): | ||
| try: | ||
| content = netperf_base.ssh_cmd(clients[-1], "cat %s" % fname) | ||
| except: | ||
| content = "" | ||
| return False | ||
| if int(sessions) == len(re.findall("MIGRATE", content)): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Bare except clause hides errors.
The bare except at line 697 catches all exceptions including KeyboardInterrupt and SystemExit, which can mask real issues. Catch specific exceptions or at minimum Exception.
🔎 Proposed fix
def all_clients_up():
try:
content = netperf_base.ssh_cmd(clients[-1], "cat %s" % fname)
- except:
+ except Exception:
content = ""
return False🧰 Tools
🪛 Ruff (0.14.10)
697-697: Do not use bare except
(E722)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/netperf.py around lines 694 to 702,
replace the bare except that currently swallows all exceptions when running
netperf_base.ssh_cmd with a specific exception handler (e.g., except (IOError,
OSError, netperf_base.SshError) as e: or at minimum except Exception as e:) so
that KeyboardInterrupt/SystemExit are not caught; assign content = "" and return
False inside that handler and optionally log or re-raise other unexpected
exceptions as needed.
| 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.
| 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.
Bug: Unused return value and incorrect function call.
Line 401 calls params.get() but discards the result. More critically, line 404 calls install_driver_by_virtio_media with only 4 arguments, but the function signature at line 159 requires 6 arguments (session, test, devcon_path, media_type, driver_name, device_hwid).
🔎 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)
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.
| 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 to 404, the code
discards the return value of params.get("virtio_win_media_type", "iso") and
calls install_driver_by_virtio_media with only 4 arguments while the function
defined at line 159 expects 6 (session, test, devcon_path, media_type,
driver_name, device_hwid). Fix by assigning the params.get(...) result to a
variable (e.g., media_type = params.get("virtio_win_media_type", "iso")), ensure
driver_name is available/used, then call install_driver_by_virtio_media(session,
test, devcon_path, media_type, driver_name, device_hwid).
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.
| 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:
find . -name "netperf_base.py" -o -name "netperf.py" 2>/dev/null | head -20Repository: autotest/tp-libvirt
Length of output: 157
🏁 Script executed:
# Examine netperf_thread function in netperf_base.py
cat -n ./provider/virtual_network/netperf_base.py | sed -n '85,105p'Repository: autotest/tp-libvirt
Length of output: 728
🏁 Script executed:
# Examine the caller in netperf.py around line 691
cat -n ./libvirt/tests/src/virtual_network/qemu/netperf.py | sed -n '680,700p'Repository: autotest/tp-libvirt
Length of output: 926
🏁 Script executed:
# Look for all netperf_thread calls to understand context
rg -n "netperf_thread" --type py -B2 -A2 ./libvirt/tests/src/virtual_network/qemu/netperf.pyRepository: autotest/tp-libvirt
Length of output: 236
🏁 Script executed:
# Verify the exact flow by looking at more context around the function definition
cat -n ./provider/virtual_network/netperf_base.py | sed -n '88,101p'Repository: autotest/tp-libvirt
Length of output: 558
Remove redundant file redirection in netperf_thread caller.
The option parameter passed to netperf_thread at line 691 in netperf.py already includes >> %s redirection, but netperf_thread appends the same redirection again at line 98 of netperf_base.py, causing double redirection. Either remove the redirection from the option string construction in netperf.py, or remove line 98 from the netperf_thread function to avoid appending it again.
🤖 Prompt for AI Agents
In provider/virtual_network/netperf_base.py around lines 88 to 100,
netperf_thread appends " >> %s" % fname to the constructed command but the
caller already includes that redirection, resulting in double redirection;
remove the line that appends the file redirection (the cmd += " >> %s" % fname
statement) so netperf_thread uses the option string as provided, and keep the
numa handling and logging intact.
| :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.
| 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.
Potential AttributeError if version regex doesn't match.
If cdrom_virtio_path doesn't match the pattern virtio-win-(\d+\.\d+(?:\.\d+)?-\d+), the match variable will be None, and calling match.group(1) will raise an AttributeError.
🔎 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))📝 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.
| match = re.search(r"virtio-win-(\d+\.\d+(?:\.\d+)?-\d+)", cdrom_virtio_path) | |
| cdrom_virtio_version = re.sub("-", ".", match.group(1)) | |
| 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)) |
🤖 Prompt for AI Agents
In provider/windows/win_driver_utils.py around lines 242 to 243, the regex
search result may be None and calling match.group(1) will raise an
AttributeError; update the code to check whether match is truthy before using
group(1) and handle the no-match case explicitly (either raise a clear
ValueError with context including cdrom_virtio_path, or set cdrom_virtio_version
to a sensible default and log a warning) so the function fails with a
descriptive error or recovers gracefully instead of throwing AttributeError.
| 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.
Potential IndexError if no device found.
If _pnpdrv_info returns an empty list (no devices matching device_name), accessing output[0] will raise an IndexError.
🔎 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]📝 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 matching '%s'" % 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 to 378, the code
assumes _pnpdrv_info(...) returns a non-empty list and directly indexes
output[0], which can raise IndexError if no device was found; update the code to
check if output is empty before accessing output[0], and handle the empty case
by either raising a clear, descriptive exception (including the device_name and
context) or returning None/False as appropriate for the calling code, and only
perform the "&" replacement when a valid device_id string is present.
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. 2. netperf.py: removed Cygwin/VM-to-VM scenario and use pin functions from network module. 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 <wquan@redhat.com>
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)
1 participant
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:
ID: LIBVIRTAT-22217
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.