Add case for enable scantter-gather offload Tx guest #6741
Conversation
WalkthroughAdds a Windows-only, virtio_net-specific test configuration at libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg and a new test module libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py exporting run(test, params, env). The config sets VM lifecycle, timeouts, images, and capture/tooling requirements. The test boots a Windows guest, manages/install/verifies capture tooling (WinPcap, windump, Wireshark/tshark, AutoIt), queries the Red Hat VirtIO Ethernet driver, toggles netkvm offload/scatter-gather parameters, orchestrates PCAP captures (architecture-specific), parses captures to validate scatter-gather behavior, and performs process checks and cleanup. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (5)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg (1)
1-1: Minor: Trailing whitespace after test name.The test name on line 1 has trailing spaces which should be removed for cleaner config.
-- virtual_network.qemu_test.enable_scatter_windows: ++ virtual_network.qemu_test.enable_scatter_windows:libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (4)
113-113: Consider making the sleep duration configurable.The hardcoded 30-second sleep could be parameterized for flexibility in different test environments.
+ transfer_wait_time = params.get_numeric("transfer_wait_time", 30) ... - time.sleep(30) + time.sleep(transfer_wait_time)
156-159: Remove commented-out code.The commented-out
vm.send_keycall on line 157 is dead code. Either use it or remove it entirely to keep the codebase clean.session = vm.wait_for_login(timeout=timeout) # make sure to enter desktop - #vm.send_key("meta_l-d") virsh.sendkey(vm_name, "KEY_LEFTMETA KEY_D", ignore_status=True) time.sleep(30)
205-210: Bind loop variable in lambda to avoid late binding issues.The static analysis tool correctly flags that the lambda doesn't bind
process_name. While the current code works becausewait_foris called synchronously within the loop iteration, this pattern is fragile and could break if refactored.🔎 Recommended fix
utils_misc.wait_for( - lambda: _is_process_finished(session, process_name), + lambda pn=process_name: _is_process_finished(session, pn), timeout, 20, 3, )
228-231: Verification message inconsistency with filter.The log message on line 229 says "Check length > 1514 packets" but the failure message on line 231 says "No packet length >= 1514". The filter actually checks
frame.len>1514(strictly greater than). Consider aligning the messages for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🧰 Additional context used
🧠 Learnings (2)
📚 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/enable_scatter_windows.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/enable_scatter_windows.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (1)
provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
206-206: Function definition does not bind loop variable process_name
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (7)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg (2)
33-34: Intentional asymmetry between param_names and param_values confirmed.The config defines 2
param_names("Offload.TxLSO Offload.TxLSO") and 3param_values("1 0 1000"). This is intentional because the Python code dynamically appends a third parameter name (*JumboPacketorMTU) based on driver version at runtime (lines 167-170 in the .py file). The three param_values map to: enable TxLSO (1), disable TxLSO (0), and set MTU/JumboPacket (1000).
22-28: Consider adding 32-bit path variant for tshark.The
tshark_installed_pathhardcodesC:\Program Files\Wireshark\tshark.exewhich is the 64-bit path. On 32-bit Windows (i386/i686), Wireshark installs toC:\Program Files (x86)\Wireshark\. The architecture-specific handling forwireshark_nameexists but the installed path doesn't follow suit.Verify if the test is expected to run on 32-bit Windows guests. If so, consider:
x86_64: wireshark_name = "Wireshark-win64-1.10.1.exe" + tshark_installed_path = "C:\Program Files\Wireshark\tshark.exe" i386, i686: wireshark_name = "Wireshark-win32-1.10.1.exe" -tshark_installed_path = "C:\Program Files\Wireshark\tshark.exe" + tshark_installed_path = "C:\Program Files (x86)\Wireshark\tshark.exe"libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (5)
44-72: LGTM!The function properly manages the serial session lifecycle with a
finallyblock, and the wait logic correctly verifies that WinDump has started before proceeding.
74-83: LGTM!Simple and effective session termination.
85-100: LGTM!Proper error handling for tshark parsing.
238-244: Missing session cleanup at end of test.The session opened at line 238 is never closed. Add
session.close()at the end or use a try/finally block for proper resource cleanup.🔎 Suggested fix
session = vm.wait_for_login(timeout=timeout) error_context.context( "Log network traffic with scatter gather disabled", test.log.info ) test.log.info("Check length > 1014 packets") output = _get_traffic_log("frame.len>1014") if "Len" in output: test.fail("Some packet length > 1014, output=%s" % output) + session.close()⛔ Skipped due to learnings
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.
29-42: Process detection logic may be inverted when status is non-zero.Windows tasklist with /FI filters always returns exit code 0; the actual exit code typically comes from a piped command like find. If this code pipes tasklist output to find (or similar), a non-zero exit code indicates the process was NOT found, meaning the function should return
True(finished), notFalse.Current logic at line 40-41 returns
Falsewhen status is non-zero, but this contradicts the expected behavior where non-zero status means process absence.
yiqianwei
force-pushed
the
add_case_for_enable_scatter_windows
branch
from
c9101e4 to
9e71d90
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (1)
127-136: Add defensive parsing for driver version extraction.The version parsing can raise
IndexErrororValueErrorif the WMI output format differs from expected (e.g., empty output, missing "=" separator, or non-numeric version components).🔎 Suggested defensive implementation
def _get_driver_version(session): """ Get current installed virtio driver version return: a int value of version, e.g. 191 """ query_version_cmd = params["query_version_cmd"] output = session.cmd_output(query_version_cmd) - version_str = output.strip().split("=")[1] - version = version_str.split(".")[-1][0:3] - return int(version) + try: + version_str = output.strip().split("=")[1] + version = version_str.split(".")[-1][0:3] + return int(version) + except (IndexError, ValueError) as e: + test.error("Failed to parse driver version from output: %s, error: %s" + % (output, e))
🧹 Nitpick comments (4)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (4)
117-125: Consider adding bounds validation.While the hardcoded indices (0, 1, 2) used by callers should be safe given the configuration, adding defensive bounds checking would make the function more robust against configuration errors.
🔎 Optional defensive implementation
def _set_driver_param(index): """ set the netkvm driver param's value. param index: the index of the list of target params """ + if index >= len(param_names) or index >= len(param_values): + test.error("Invalid param index %d (param_names=%d, param_values=%d)" + % (index, len(param_names), len(param_values))) param_name = param_names[index] param_value = param_values[index] utils_net.set_netkvm_param_value(vm, param_name, param_value)
178-182: Remove duplicate parameter retrieval.
autoit_nameis already retrieved at line 140. The retrieval at line 179 is redundant.🔎 Suggested fix
test.log.info("Wait for pcap installation to complete") -autoit_name = params.get("autoit_name") utils_misc.wait_for( lambda: _is_process_finished(session, autoit_name), timeout, 20, 3 )
184-216: Optional: silence linter warning for loop variable capture.The static analysis tool flags line 205 for capturing the loop variable
process_namein a lambda. While the code works correctly (the lambda is evaluated immediately within the same iteration), you can silence the warning using a default argument pattern.🔎 Optional fix to silence linter
error_context.context( "Wait for %s installation to complete" % process_name, test.log.info ) utils_misc.wait_for( - lambda: _is_process_finished(session, process_name), + lambda pname=process_name: _is_process_finished(session, pname), timeout, 20, 3, )
237-244: Clarify the validation logic.The comment at line 241 says "Check length > 1014 packets" but the code actually verifies that NO packets exceed 1014 bytes. Consider updating the comment for clarity.
🔎 Suggested improvement
session = vm.wait_for_login(timeout=timeout) error_context.context( "Log network traffic with scatter gather disabled", test.log.info ) -test.log.info("Check length > 1014 packets") +test.log.info("Check that no packets exceed 1014 bytes") output = _get_traffic_log("frame.len>1014") if "Len" in output: test.fail("Some packet length > 1014, output=%s" % output)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.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.
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (2)
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)
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
205-205: Function definition does not bind loop variable process_name
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (7)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (7)
1-7: LGTM!The imports are appropriate for a Windows guest networking test. All necessary modules for VM management, network utilities, and Windows-specific virtio handling are included.
9-27: LGTM!The test entry point follows the standard libvirt test framework pattern with proper decoration and clear documentation of the test workflow.
29-42: LGTM!The process check logic is sound with a safe default behavior: if the status check command fails, it assumes the process still exists, preventing premature continuation.
44-72: LGTM!The WinDump session startup is well-structured with proper resource management (session closed in finally block) and appropriate use of serial console for background process management.
74-83: LGTM!The stop function is straightforward with appropriate error handling.
85-100: LGTM!The log parsing function is clean and properly handles errors by calling test.error on failure.
102-115: LGTM!The traffic capture workflow is logical. The 30-second sleep allows adequate time for file transfer traffic to be captured before stopping WinDump.
| test.log.info("Check length > 1514 packets") | ||
| if "Len" not in output: | ||
| test.fail("No packet length >= 1514, output=%s" % output) |
There was a problem hiding this comment.
Fix error message inconsistency.
The filter checks for frame.len>1514 (strictly greater than), but the error message says "No packet length >= 1514". The message should match the filter condition.
🔎 Suggested fix
test.log.info("Check length > 1514 packets")
if "Len" not in output:
- test.fail("No packet length >= 1514, output=%s" % output)
+ test.fail("No packet length > 1514, output=%s" % output)📝 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.
| test.log.info("Check length > 1514 packets") | |
| if "Len" not in output: | |
| test.fail("No packet length >= 1514, output=%s" % output) | |
| test.log.info("Check length > 1514 packets") | |
| if "Len" not in output: | |
| test.fail("No packet length > 1514, output=%s" % output) |
🤖 Prompt for AI Agents
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py around lines
228 to 230: the filter used is frame.len>1514 (strictly greater), but the error
text says "No packet length >= 1514"; update the test.fail call to match the
strict comparison (e.g., "No packet length > 1514, output=%s" % output) so the
message accurately reflects the filter condition.
yiqianwei
force-pushed
the
add_case_for_enable_scatter_windows
branch
from
9e71d90 to
1865d9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (3)
127-136: Add defensive parsing for driver version extraction.This issue was already flagged in a previous review. The version parsing at lines 134-135 can raise
IndexErrororValueErrorif the WMI output format differs from expected.
154-157: Remove commented-out code.This issue was already flagged in a previous review. Line 156 contains commented-out code that should be removed.
227-229: Fix error message inconsistency.This issue was already flagged in a previous review. The filter checks for
frame.len>1514(strictly greater than), but the error message says ">= 1514".
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (1)
178-178: Remove redundant variable assignment.
autoit_nameis already retrieved and assigned at line 140, making this reassignment unnecessary.🔎 Proposed cleanup
test.log.info("Wait for pcap installation to complete") -autoit_name = params.get("autoit_name") utils_misc.wait_for( lambda: _is_process_finished(session, autoit_name), timeout, 20, 3 )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🧰 Additional context used
🧠 Learnings (6)
📓 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/enable_scatter_windows.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/enable_scatter_windows.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/enable_scatter_windows.py
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
📚 Learning: 2025-11-18T01:20:50.891Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6654
File: libvirt/tests/src/virtual_network/qemu/boot_with_multiqueue.py:56-58
Timestamp: 2025-11-18T01:20:50.891Z
Learning: In the tp-libvirt project, when calling session command methods and the status code is not needed, prefer using `session.cmd_output()` over `session.cmd_status_output()` to avoid unused variables.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (2)
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)
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
204-204: Function definition does not bind loop variable process_name
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
🔇 Additional comments (4)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (4)
29-42: LGTM!The process check logic correctly handles both command execution status and output parsing.
44-72: LGTM!Proper session management with error handling and timeout verification for windump startup.
74-100: LGTM!Both functions implement proper error handling and status validation.
102-125: LGTM!Clean orchestration of traffic capture and parameter configuration.
| param_names = "Offload.TxLSO Offload.TxLSO" | ||
| param_values = "1 0 1000" |
There was a problem hiding this comment.
Fix duplicate parameter name causing incorrect driver configuration.
Line 33 contains duplicate Offload.TxLSO entries. Based on the Python test logic (lines 220, 233-234), three distinct parameters are expected:
- Index 0: Enable scatter-gather (Offload.TxLSO = 1)
- Index 1: Disable scatter-gather (Offload.TxLSO = 0)
- Index 2: Set packet size limit (JumboPacket/MTU = 1000)
The second Offload.TxLSO should be Offload.TxLSO (for disabling), making it:
🔎 Proposed fix
-param_names = "Offload.TxLSO Offload.TxLSO"
+param_names = "Offload.TxLSO Offload.TxLSO"Wait, reviewing more carefully: the test enables TxLSO first (index 0, value 1), then disables it (index 1, value 0), then sets size limit (index 2, value 1000). So this should be:
-param_names = "Offload.TxLSO Offload.TxLSO"
+param_names = "Offload.TxLSO Offload.TxLSO"Actually, the pattern suggests the first two operations target the same parameter with different values. Please verify the correct parameter names for indices 0 and 1, ensuring they match the intended enable/disable sequence.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg around
lines 33-34 the param_names line has only two entries while param_values has
three, and the test expects three parameters (enable TxLSO, disable TxLSO, then
set packet size). Update param_names to contain three corresponding entries
(e.g., "Offload.TxLSO Offload.TxLSO JumboPacket" or the correct MTU parameter
name used elsewhere) so it matches the three values "1 0 1000", ensuring the
second name intentionally targets disabling TxLSO and the third is the packet
size parameter.
| pcap_file = c:\temp.pcap | ||
| run_windump_temp = 'start "" "${windump_installed_path}" -n -w "${pcap_file}" tcp and dst %s and src %s' | ||
| stop_windump_cmd = "taskkill /im ${windump_name} /f" | ||
| parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -R "%s"' |
There was a problem hiding this comment.
Use -Y flag for forward compatibility.
The -R flag for tshark became deprecated in Wireshark 2.0+. While Wireshark 1.10.1 specified in the config supports -R, using -Y ensures compatibility with newer versions where -Y is the standard for display filtering.
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg around line
42, the tshark command uses the deprecated -R flag; replace -R with -Y in the
parse_log_temp string to ensure forward compatibility with Wireshark 2.0+
(preserve the rest of the string, quoting and formatting unchanged so the
display filter is passed exactly as before).
| for process_name in ["windump", "tshark"]: | ||
| check_cmd = params["check_%s_installed_cmd" % process_name] | ||
| install_cmd = utils_misc.set_winutils_letter( | ||
| session, params["%s_install_cmd" % process_name] | ||
| ) | ||
|
|
||
| is_installed = process_name in session.cmd_output(check_cmd) | ||
|
|
||
| if not is_installed: | ||
| error_context.context("Install %s" % process_name, test.log.info) | ||
| status, output = session.cmd_status_output(install_cmd, timeout=timeout) | ||
| if status != 0: | ||
| test.error( | ||
| "Failed to install %s, status=%s, output=%s" | ||
| % (process_name, status, output) | ||
| ) | ||
|
|
||
| error_context.context( | ||
| "Wait for %s installation to complete" % process_name, test.log.info | ||
| ) | ||
| utils_misc.wait_for( | ||
| lambda: _is_process_finished(session, process_name), | ||
| timeout, | ||
| 20, | ||
| 3, | ||
| ) | ||
| check_result = session.cmd_output(check_cmd).lower() | ||
| if process_name not in check_result: | ||
| test.error("%s installation failed to verify." % process_name) | ||
| test.log.info("%s installed successfully.", process_name) | ||
| else: | ||
| test.log.info("%s is already installed", process_name) |
There was a problem hiding this comment.
Fix lambda variable capture in installation verification loop.
The lambda at line 204 captures the loop variable process_name by reference, which can cause late-binding issues where the lambda always references the last value of process_name from the loop iteration.
🔎 Proposed fix using default argument
error_context.context(
"Wait for %s installation to complete" % process_name, test.log.info
)
utils_misc.wait_for(
- lambda: _is_process_finished(session, process_name),
+ lambda pname=process_name: _is_process_finished(session, pname),
timeout,
20,
3,
)Alternatively, extract the process name before the lambda:
error_context.context(
"Wait for %s installation to complete" % process_name, test.log.info
)
+ current_process = process_name
utils_misc.wait_for(
- lambda: _is_process_finished(session, process_name),
+ lambda: _is_process_finished(session, current_process),
timeout,
20,
3,
)Based on static analysis hints.
📝 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.
| for process_name in ["windump", "tshark"]: | |
| check_cmd = params["check_%s_installed_cmd" % process_name] | |
| install_cmd = utils_misc.set_winutils_letter( | |
| session, params["%s_install_cmd" % process_name] | |
| ) | |
| is_installed = process_name in session.cmd_output(check_cmd) | |
| if not is_installed: | |
| error_context.context("Install %s" % process_name, test.log.info) | |
| status, output = session.cmd_status_output(install_cmd, timeout=timeout) | |
| if status != 0: | |
| test.error( | |
| "Failed to install %s, status=%s, output=%s" | |
| % (process_name, status, output) | |
| ) | |
| error_context.context( | |
| "Wait for %s installation to complete" % process_name, test.log.info | |
| ) | |
| utils_misc.wait_for( | |
| lambda: _is_process_finished(session, process_name), | |
| timeout, | |
| 20, | |
| 3, | |
| ) | |
| check_result = session.cmd_output(check_cmd).lower() | |
| if process_name not in check_result: | |
| test.error("%s installation failed to verify." % process_name) | |
| test.log.info("%s installed successfully.", process_name) | |
| else: | |
| test.log.info("%s is already installed", process_name) | |
| for process_name in ["windump", "tshark"]: | |
| check_cmd = params["check_%s_installed_cmd" % process_name] | |
| install_cmd = utils_misc.set_winutils_letter( | |
| session, params["%s_install_cmd" % process_name] | |
| ) | |
| is_installed = process_name in session.cmd_output(check_cmd) | |
| if not is_installed: | |
| error_context.context("Install %s" % process_name, test.log.info) | |
| status, output = session.cmd_status_output(install_cmd, timeout=timeout) | |
| if status != 0: | |
| test.error( | |
| "Failed to install %s, status=%s, output=%s" | |
| % (process_name, status, output) | |
| ) | |
| error_context.context( | |
| "Wait for %s installation to complete" % process_name, test.log.info | |
| ) | |
| utils_misc.wait_for( | |
| lambda pname=process_name: _is_process_finished(session, pname), | |
| timeout, | |
| 20, | |
| 3, | |
| ) | |
| check_result = session.cmd_output(check_cmd).lower() | |
| if process_name not in check_result: | |
| test.error("%s installation failed to verify." % process_name) | |
| test.log.info("%s installed successfully.", process_name) | |
| else: | |
| test.log.info("%s is already installed", process_name) |
🧰 Tools
🪛 Ruff (0.14.8)
204-204: Function definition does not bind loop variable process_name
(B023)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py around lines
183-214, the lambda passed to utils_misc.wait_for captures the loop variable
process_name by reference causing late-binding; fix it by binding the current
loop value into the lambda (for example: lambda pn=process_name:
_is_process_finished(session, pn)) or by assigning a local variable (e.g.,
current_process = process_name) and using lambda: _is_process_finished(session,
current_process) so each iteration checks the correct process.
1865d9f to
9ddf68a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (4)
127-136: Defensive parsing still needed.As noted in the previous review, this function lacks error handling for malformed WMI output.
154-157: Commented code removal still pending.As noted in the previous review, the commented line should be removed.
183-214: Lambda variable capture issue still needs fixing.As noted in the previous review, the lambda at line 204 has a late-binding variable capture issue.
227-229: Error message inconsistency still present.As noted in the previous review, the error message should say ">" not ">=" to match the filter condition.
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (2)
178-178: Remove redundant variable assignment.The variable
autoit_nameis already retrieved at line 140, making this assignment redundant.🔎 Proposed fix
- test.log.info("Wait for pcap installation to complete") - autoit_name = params.get("autoit_name") - utils_misc.wait_for( + test.log.info("Wait for pcap installation to complete") + utils_misc.wait_for( lambda: _is_process_finished(session, autoit_name), timeout, 20, 3 )
236-243: Add missing session cleanup.The session opened at line 236 is never closed before the function exits, creating a potential resource leak.
🔎 Proposed fix
session = vm.wait_for_login(timeout=timeout) error_context.context( "Log network traffic with scatter gather disabled", test.log.info ) test.log.info("Check length > 1014 packets") output = _get_traffic_log("frame.len>1014") if "Len" in output: test.fail("Some packet length > 1014, output=%s" % output) + session.close()Based on learnings, VM cleanup is handled by the test framework's teardown method. However, explicit session cleanup is still considered best practice for resource management.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg
🧰 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.
📚 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/enable_scatter_windows.py
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
📚 Learning: 2025-11-18T01:20:50.891Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6654
File: libvirt/tests/src/virtual_network/qemu/boot_with_multiqueue.py:56-58
Timestamp: 2025-11-18T01:20:50.891Z
Learning: In the tp-libvirt project, when calling session command methods and the status code is not needed, prefer using `session.cmd_output()` over `session.cmd_status_output()` to avoid unused variables.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
204-204: Function definition does not bind loop variable process_name
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
🔇 Additional comments (5)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (5)
1-27: LGTM!The imports and test documentation are clear and appropriate. The test flow is well-documented.
29-42: LGTM!The process check logic is sound and handles both command failure and output validation.
44-72: LGTM!The WinDump session management correctly uses try/finally for cleanup, and the wait_for logic properly verifies process startup before the session closes.
74-100: LGTM!Both helper functions have appropriate error handling and clear context logging.
102-125: LGTM!The traffic logging orchestration and driver parameter setting are implemented correctly.
|
@yanglei-rh and @nanli1 , Could you help to review this patch, thanks |
| kill_vm_libvirt_options = --nvram | ||
| filesize = 50 | ||
| clean_cmd = del | ||
| image_snapshot = yes |
There was a problem hiding this comment.
This parameter is used by qemu level, please remove it. And this case change the nic device, if you would like implement image backup, please use the following parameters:
master_images_clone = img1
remove_image_image1 = yes
There was a problem hiding this comment.
I have update it to cfg file, please check it,thanks
|
|
||
| check_proc_temp = 'tasklist /fi "IMAGENAME eq %s"' | ||
| driver_verifier = netkvm | ||
| Win2016, Win2019, Win8..1, Win2012..r2: |
There was a problem hiding this comment.
Win2012..r2 and Win8..1 no longer needs testing, please delete.
There was a problem hiding this comment.
okay, I have removed it, please check it
| windump_install_cmd = "xcopy WIN_UTILS:\${windump_name} c:\ /y" | ||
| x86_64: | ||
| wireshark_name = "Wireshark-win64-1.10.1.exe" | ||
| i386, i686: |
| stop_windump_cmd = params.get("stop_windump_cmd") | ||
| check_proc_temp = params.get("check_proc_temp") | ||
| parse_log_temp = params.get("parse_log_temp") | ||
| param_names = params.get("param_names").split() |
There was a problem hiding this comment.
params.get_list() for param_names and param_values.
There was a problem hiding this comment.
Okay, I have updated it
| run_windump_temp = 'start "" "${windump_installed_path}" -n -w "${pcap_file}" tcp and dst %s and src %s' | ||
| stop_windump_cmd = "taskkill /im ${windump_name} /f" | ||
| parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -R "%s"' | ||
|
|
| session, vm, test, driver_verifier, timeout | ||
| ) | ||
|
|
||
| if _get_driver_version(session) > 189: |
There was a problem hiding this comment.
For this part, I think we don't need to consider the old driver. Only focus on the latest version is enough.
There was a problem hiding this comment.
Agree， remove old driver
|
Please don't forget to delete all the extra blank line and space. And confirm all the code style are following the python. |
yiqianwei
force-pushed
the
add_case_for_enable_scatter_windows
branch
from
9ddf68a to
1cb0d42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg (1)
39-39: Consider using-Yflag for forward compatibility.The
-Rflag was deprecated in Wireshark 2.0+ and replaced with-Yfor display filters. While Wireshark 1.10.1 (specified in line 25) supports-R, using-Yensures compatibility with newer versions:-parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -R "%s"' +parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -Y "%s"'libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (2)
229-234: Fix lambda variable capture in loop.The lambda at line 230 captures the loop variable
process_nameby reference, which can cause late-binding issues. While the risk is low here sincewait_for()executes immediately, it's best practice to bind the current value:🔎 Proposed fix
utils_misc.wait_for( - lambda: _is_process_finished(session, process_name), + lambda pname=process_name: _is_process_finished(session, pname), timeout, 20, 3, )Based on static analysis hints.
253-255: Fix error message to match filter condition.The filter uses
frame.len>1514(strictly greater), but the error message says>= 1514:🔎 Proposed fix
test.log.info("Check length > 1514 packets") if "Len" not in output: - test.fail("No packet length >= 1514, output=%s" % output) + test.fail("No packet length > 1514, output=%s" % output)
🧹 Nitpick comments (1)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg (1)
31-32: Add third parameter name to config for clarity.Currently,
param_nameshas 2 entries whileparam_valueshas 3. The Python test (line 195) appends"*JumboPacket"to reconcile the count, but this split between config and code is confusing.Consider defining all 3 parameter names directly in the config:
-param_names = "Offload.TxLSO Offload.TxLSO" +param_names = "Offload.TxLSO Offload.TxLSO *JumboPacket" param_values = "1 0 1000"Then remove the
param_names.append("*JumboPacket")line from the Python file (line 195).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🧰 Additional context used
🧠 Learnings (6)
📓 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-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/enable_scatter_windows.py
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
📚 Learning: 2025-11-18T01:20:50.891Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6654
File: libvirt/tests/src/virtual_network/qemu/boot_with_multiqueue.py:56-58
Timestamp: 2025-11-18T01:20:50.891Z
Learning: In the tp-libvirt project, when calling session command methods and the status code is not needed, prefer using `session.cmd_output()` over `session.cmd_status_output()` to avoid unused variables.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.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/cfg/virtual_network/qemu/enable_scatter_windows.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/enable_scatter_windows.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (4)
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)libvirt/tests/src/virtual_disks/virtual_disks_dasd.py (1)
attach_disk(98-108)libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (1)
extra(77-79)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
169-169: Do not catch blind exception: Exception
(BLE001)
182-182: Do not catch blind exception: Exception
(BLE001)
230-230: Function definition does not bind loop variable process_name
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
| def _get_driver_version(session): | ||
| """ | ||
| Get current installed virtio driver version | ||
| return: a int value of version, e.g. 191 | ||
| """ | ||
| query_version_cmd = params["query_version_cmd"] | ||
| output = session.cmd_output(query_version_cmd) | ||
| version_str = output.strip().split("=")[1] | ||
| version = version_str.split(".")[-1][0:3] | ||
| return int(version) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused function.
The _get_driver_version function is defined but never called in the test. Based on the maintainer's feedback to focus only on the latest driver version, this version detection logic is unnecessary.
🔎 Proposed removal
- def _get_driver_version(session):
- """
- Get current installed virtio driver version
- return: a int value of version, e.g. 191
- """
- query_version_cmd = params["query_version_cmd"]
- output = session.cmd_output(query_version_cmd)
- version_str = output.strip().split("=")[1]
- version = version_str.split(".")[-1][0:3]
- return int(version)
-📝 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 _get_driver_version(session): | |
| """ | |
| Get current installed virtio driver version | |
| return: a int value of version, e.g. 191 | |
| """ | |
| query_version_cmd = params["query_version_cmd"] | |
| output = session.cmd_output(query_version_cmd) | |
| version_str = output.strip().split("=")[1] | |
| version = version_str.split(".")[-1][0:3] | |
| return int(version) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py around lines
129 to 138, the helper function _get_driver_version is defined but never used;
remove the entire function definition (including its docstring and internal
logic) to eliminate dead code and update any imports or references if they exist
elsewhere; run the test suite/linters to ensure no remaining references and
commit the deletion.
|
Test results: |
yiqianwei
force-pushed
the
add_case_for_enable_scatter_windows
branch
from
1cb0d42 to
c3e3658
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg (2)
39-39: Consider using-Yflag for forward compatibility.The
-Rflag for tshark was deprecated in Wireshark 2.0+ in favor of-Y. While Wireshark 1.10.1 specified in line 25 supports-R, using-Ywould ensure compatibility with newer versions.🔎 Proposed fix
- parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -R "%s"' + parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -Y "%s"'
31-32: Add missing third parameter name to match parameter values.Line 31 defines only 2 parameter names but line 32 defines 3 values. The test code works around this by appending
"*JumboPacket"at runtime (line 185 in the Python file), which is poor design. All parameter names should be defined in the configuration file.🔎 Proposed fix
- param_names = "Offload.TxLSO Offload.TxLSO" + param_names = "Offload.TxLSO Offload.TxLSO *JumboPacket" param_values = "1 0 1000"Then remove line 185 from the Python test file to eliminate the runtime workaround.
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (2)
199-230: Fix lambda variable capture in loop.Line 220 captures the loop variable
process_nameby reference in the lambda, which can cause late-binding issues. While it may work in this case becauseutils_misc.wait_forevaluates the lambda immediately, it's a code smell flagged by static analysis.🔎 Proposed fix
error_context.context( "Wait for %s installation to complete" % process_name, test.log.info ) utils_misc.wait_for( - lambda: _is_process_finished(session, process_name), + lambda pn=process_name: _is_process_finished(session, pn), timeout, 20, 3, )Based on static analysis hints.
242-245: Fix error message to match filter condition.Line 242 uses filter
frame.len>1514(strictly greater than), but line 245 says"No packet length >= 1514". The message should match the filter.🔎 Proposed fix
test.log.info("Check length > 1514 packets") if "Len" not in output: - test.fail("No packet length >= 1514, output=%s" % output) + test.fail("No packet length > 1514, output=%s" % output)
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (1)
148-173: Consider catching specific exceptions.Lines 159 and 172 catch bare
Exception. While the test can continue without these CDROMs (as they log warnings), catching specific exceptions (e.g.,virsh.LibvirtErroror more specific error types) would make the error handling more precise.🔎 Example refactor
try: virsh.attach_disk(vm.name, winutils_path, "sdc", extra="--type cdrom --mode readonly", debug=True, ignore_status=False) test.log.info("Successfully attached winutils CDROM") - except Exception as e: + except (virsh.LibvirtError, OSError) as e: test.log.warning("Failed to attach winutils CDROM: %s", e)Apply similar change to the virtio-win CDROM attachment block.
Based on static analysis hints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🧰 Additional context used
🧠 Learnings (6)
📓 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-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/enable_scatter_windows.py
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
📚 Learning: 2025-11-18T01:20:50.891Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6654
File: libvirt/tests/src/virtual_network/qemu/boot_with_multiqueue.py:56-58
Timestamp: 2025-11-18T01:20:50.891Z
Learning: In the tp-libvirt project, when calling session command methods and the status code is not needed, prefer using `session.cmd_output()` over `session.cmd_status_output()` to avoid unused variables.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.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/cfg/virtual_network/qemu/enable_scatter_windows.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/enable_scatter_windows.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (3)
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)libvirt/tests/src/virtual_disks/virtual_disks_dasd.py (1)
attach_disk(98-108)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
159-159: Do not catch blind exception: Exception
(BLE001)
172-172: Do not catch blind exception: Exception
(BLE001)
220-220: Function definition does not bind loop variable process_name
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
| session, vm, test, driver_verifier, timeout | ||
| ) | ||
|
|
||
| param_names.append("*JumboPacket") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move hardcoded parameter name to configuration file.
Appending "*JumboPacket" directly in the test code couples the test logic to a hardcoded parameter name. This should be defined in the config file alongside the other parameter names to maintain separation of test logic and configuration.
Move this parameter to the config file:
In libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg, update line 31:
param_names = "Offload.TxLSO Offload.TxLSO *JumboPacket"
Then remove line 185 from the Python test.
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py around line
185, the test currently appends the hardcoded parameter name "*JumboPacket";
remove that hardcoded append from the Python test and instead add "*JumboPacket"
to the configuration file
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg by updating
line 31’s param_names to include "*JumboPacket" (e.g. param_names =
"Offload.TxLSO Offload.TxLSO *JumboPacket"); ensure the test reads parameter
names from the config as the others do and does not reference the hardcoded
string.
xxx-95906 - [virtual network][virtual-nic-device] enable scatter-gather offload Tx guest ID: 129431 Signed-off-by: Yiqian Wei <yiwei@redhat.com>
yiqianwei
force-pushed
the
add_case_for_enable_scatter_windows
branch
from
c3e3658 to
9c3d269
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfg (2)
39-39: Use-Yflag for forward compatibility.The
-Rflag for tshark was deprecated in Wireshark 2.0+ and replaced with-Y. While the configured Wireshark 1.10.1 supports-R, using-Yensures compatibility with newer versions.🔎 Proposed fix
-parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -R "%s"' +parse_log_temp = '"${tshark_installed_path}" -2 -r "${pcap_file}" -Y "%s"'
31-32: Fix parameter configuration mismatch.The
param_namescontains only 2 entries whileparam_valueshas 3 ("1 0 1000"). The Python test expects 3 parameters at indices 0, 1, and 2. Additionally, line 184 of the Python file hardcodes appending"*JumboPacket"toparam_names. Move this third parameter name to the configuration file to maintain proper separation of test logic and configuration.🔎 Proposed fix
-param_names = "Offload.TxLSO Offload.TxLSO" +param_names = "Offload.TxLSO Offload.TxLSO *JumboPacket" param_values = "1 0 1000"Then remove line 184 from the Python test file.
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (3)
184-184: Move hardcoded parameter to configuration file.The
"*JumboPacket"parameter name is hardcoded here but should be defined in the configuration file alongside the other parameter names. This maintains proper separation between test logic and configuration data.Update
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfgline 31:param_names = "Offload.TxLSO Offload.TxLSO *JumboPacket"Then remove this append from the Python test.
198-229: Fix lambda variable capture in loop.At line 219, the lambda captures the loop variable
process_nameby reference, which can cause late-binding issues where the lambda may reference the wrong value.🔎 Proposed fix using default argument
error_context.context( "Wait for %s installation to complete" % process_name, test.log.info ) utils_misc.wait_for( - lambda: _is_process_finished(session, process_name), + lambda pname=process_name: _is_process_finished(session, pname), timeout, 20, 3, )Based on static analysis hints.
241-244: Fix error message to match filter condition.Line 241 uses the filter
"frame.len>1514"(strictly greater than), but line 244's error message says">= 1514". The message should match the actual filter condition.🔎 Proposed fix
output = _get_traffic_log("frame.len>1514") test.log.info("Check length > 1514 packets") if "Len" not in output: - test.fail("No packet length >= 1514, output=%s" % output) + test.fail("No packet length > 1514, output=%s" % output)
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py (1)
147-172: Consider more specific exception handling.Lines 158 and 171 catch blind
Exceptionwhen attaching CDROMs. While the broad exception handling allows the test to continue gracefully (logging warnings instead of failing), consider catching more specific exceptions likeLibvirtErrororprocess.CmdErrorto avoid masking unexpected errors.🔎 Suggested refinement
try: virsh.attach_disk(vm.name, winutils_path, "sdc", extra="--type cdrom --mode readonly", debug=True, ignore_status=False) test.log.info("Successfully attached winutils CDROM") - except Exception as e: + except (process.CmdError, LibvirtError) as e: test.log.warning("Failed to attach winutils CDROM: %s", e)Apply similar changes to the virtio-win CDROM attachment at lines 167-172.
Based on static analysis hints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
🧰 Additional context used
🧠 Learnings (7)
📓 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/enable_scatter_windows.cfglibvirt/tests/src/virtual_network/qemu/enable_scatter_windows.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/enable_scatter_windows.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/enable_scatter_windows.py
📚 Learning: 2025-10-24T06:35:25.679Z
Learnt from: meinaLi
Repo: autotest/tp-libvirt PR: 6628
File: libvirt/tests/src/virtual_disks/virtual_disks_io_error.py:128-129
Timestamp: 2025-10-24T06:35:25.679Z
Learning: In libvirt tests, `virsh.snapshot_delete()` without the `--metadata` flag automatically deletes both snapshot metadata and external disk files. Explicit `os.remove()` is only needed when using `virsh.snapshot_delete()` with the `--metadata` flag, which removes only metadata.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
📚 Learning: 2025-11-18T01:20:50.891Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6654
File: libvirt/tests/src/virtual_network/qemu/boot_with_multiqueue.py:56-58
Timestamp: 2025-11-18T01:20:50.891Z
Learning: In the tp-libvirt project, when calling session command methods and the status code is not needed, prefer using `session.cmd_output()` over `session.cmd_status_output()` to avoid unused variables.
Applied to files:
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.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/enable_scatter_windows.py
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/enable_scatter_windows.py
158-158: Do not catch blind exception: Exception
(BLE001)
171-171: Do not catch blind exception: Exception
(BLE001)
219-219: Function definition does not bind loop variable process_name
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
|
@yanglei-rh , All comments have been updated. please check it,thanks |
3 participants
xxx-95906 - [virtual network][virtual-nic-device] enable scatter-gather offload Tx guest
ID: 129431
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.