Skip to content

Conversation

@nanli1
Copy link
Contributor

@nanli1 nanli1 commented Sep 8, 2025

xxxx-300100:[virtual network][virtual-nic-device]The local VM can transfer length 1473-1480 packets to remote host through UDP
Signed-off-by: nanli [email protected]


t-libvirt]# VT_TYPE=libvirt python3 /home/kar/ConfigTest.py --testcase=virtual_network.qemu_test.transfer_packets_to_remote_host_through_UDP --guestname=RHEL.10.1 --machines=q35 --nicmodel=virtio_net --platform=x86_64  --customsparams='netdst=switch' --clone=no

 (1/1) qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.1.x86_64.io-github-autotest-libvirt.virtual_network.qemu_test.transfer_packets_to_remote_host_through_UDP.q35: PASS (158.69 s)

Summary by CodeRabbit

  • Tests
    • Added a new end-to-end UDP packet transfer test: boots a VM and remote host, handles firewall differences for Linux/Windows, installs and runs netperf/netserver, generates and validates UDP traffic logs, captures network packets remotely for verification, and performs automated cleanup and error reporting.

✏️ Tip: You can customize this high-level summary in your review settings.

@nanli1 nanli1 marked this pull request as draft September 8, 2025 07:20
@nanli1 nanli1 force-pushed the add_case_for_local_VM_transfer_packets_to_remote_host_through_udp branch from 7cd07a7 to 0555697 Compare September 16, 2025 02:41
@coderabbitai
Copy link

coderabbitai bot commented Sep 16, 2025

Walkthrough

Adds a new libvirt QEMU UDP packet transfer test consisting of a config file libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg and a test script libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py. The config adds global parameters and Windows-specific overrides for firewall, netperf installation/execution, logging, and verification. The script implements an end-to-end flow: VM and remote host setup, disabling firewalls, installing and starting netserver on the remote, transferring/installing netperf on the guest, running UDP netperf, capturing traffic with tcpdump, validating netperf output and captured UDP packets, and teardown with error handling and logging.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review netperf/netserver install and run logic and command construction in .../transfer_packets_to_remote_host_through_UDP.py
  • Validate tcpdump invocation, capture file handling, and UDP packet-matching patterns
  • Check multi-session coordination and background process lifecycle (tcpdump, netserver) and related cleanup
  • Inspect firewall disablement paths and Windows vs Linux command differences
  • Verify new config fields and Windows-specific overrides in .../transfer_packets_to_remote_host_through_UDP.cfg

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a test case for UDP packet transfer from a VM to a remote host, which aligns with the PR objectives and file additions.
Docstring Coverage ✅ Passed Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@nanli1 nanli1 force-pushed the add_case_for_local_VM_transfer_packets_to_remote_host_through_udp branch 4 times, most recently from 26e3abf to b870293 Compare September 25, 2025 13:48
@nanli1 nanli1 force-pushed the add_case_for_local_VM_transfer_packets_to_remote_host_through_udp branch 5 times, most recently from 1a2ea28 to 5940466 Compare November 22, 2025 02:06
@nanli1 nanli1 marked this pull request as ready for review November 22, 2025 02:34
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (3)

225-238: Drop unused locals local_passwd and netperf_windows_path.

Both variables are assigned but never used, which adds noise and triggers F841 warnings.

-    remote_ip = params.get("remote_ip")
-    remote_user = params.get("remote_user", "root")
-    remote_passwd = params.get("remote_pwd")
-    local_passwd = params.get("local_pwd")
+    remote_ip = params.get("remote_ip")
+    remote_user = params.get("remote_user", "root")
+    remote_passwd = params.get("remote_pwd")
@@
-    netperf_pkg = params.get('netperf_pkg')
-    netperf_linux_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)
-    netperf_windows_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)
+    netperf_pkg = params.get('netperf_pkg')
+    netperf_linux_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)

154-167: Wire netperf_timeout from cfg into check_netperf_log instead of hard‑coding 120s.

The cfg already defines netperf_timeout = 120, but check_netperf_log uses a literal 120. Using the param keeps behaviour configurable while preserving the current default.

@@
-    packet_size = params.get("udp_packet_size")
-    del_log_cmd = params.get("del_log_cmd")
-    netperf_install_path = params.get("netperf_install_path")
+    packet_size = params.get("udp_packet_size")
+    netperf_timeout = int(params.get("netperf_timeout", 120))
+    del_log_cmd = params.get("del_log_cmd")
+    netperf_install_path = params.get("netperf_install_path")
@@
-        if guest_os_type == 'linux':
-            if utils_misc.wait_for(
-                    lambda: 'netperf' not in guest_session.cmd_output('pgrep -xl netperf'), 120, step=3.0):
+        if guest_os_type == 'linux':
+            if utils_misc.wait_for(
+                    lambda: 'netperf' not in guest_session.cmd_output('pgrep -xl netperf'),
+                    netperf_timeout, step=3.0):
@@
-        else:
-            cmd = 'tasklist /FI "imagename eq netperf.exe"'
-            guest_session.cmd('cd %s' % netperf_install_path)
-            if utils_misc.wait_for(lambda: not re.search(
-                    r'netperf.exe', guest_session.cmd_output(cmd)), 120, step=3.0):
+        else:
+            cmd = 'tasklist /FI "imagename eq netperf.exe"'
+            guest_session.cmd('cd %s' % netperf_install_path)
+            if utils_misc.wait_for(
+                    lambda: not re.search(r'netperf.exe', guest_session.cmd_output(cmd)),
+                    netperf_timeout, step=3.0):

Also applies to: 231-231


43-49: Consider narrowing broad except Exception handlers.

The except Exception blocks around Windows firewall calls and grep/head in verify_packet_capture are permissive and trip BLE001. For the firewall case in particular this can hide real misconfiguration. If virttest exposes a standard command/SSH exception, it would be better to catch that (or at least document why a broad catch is intentional here).

Also applies to: 198-209, 212-216

libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg (2)

7-8: Confirm whether you need variants for UDP sizes 1473–1480.

The PR description mentions testing packet sizes 1473–1480, but this cfg defines a single udp_packet_size = 1473. If the other sizes are introduced via variants elsewhere, this is fine; otherwise consider adding a variant matrix over udp_packet_size to actually cover the whole range.


25-25: Remove netperf_check_cmd unless it will be used.

netperf_check_cmd = "cd %s" is not consumed by the Python test, so it’s dead configuration and can confuse future maintainers.

-        netperf_check_cmd = "cd %s"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a302a6f and 5940466.

⛔ Files ignored due to path filters (2)
  • libvirt/tests/deps/netperf/netperf-2.7.1.tar.bz2 is excluded by !**/*.bz2
  • libvirt/tests/deps/netperf/netperf.exe is excluded by !**/*.exe
📒 Files selected for processing (2)
  • libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg (1 hunks)
  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.

Applied to files:

  • libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg
  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py
📚 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/transfer_packets_to_remote_host_through_UDP.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (1)
provider/virtual_network/network_base.py (1)
  • get_vm_ip (22-57)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py

48-48: Do not catch blind exception: Exception

(BLE001)


207-207: Do not catch blind exception: Exception

(BLE001)


215-215: Do not catch blind exception: Exception

(BLE001)


228-228: Local variable local_passwd is assigned to but never used

Remove assignment to unused variable local_passwd

(F841)


234-234: Probable insecure usage of temporary file or directory: "/tmp/UDP_tcpdump.log"

(S108)


238-238: Local variable netperf_windows_path is assigned to but never used

Remove assignment to unused variable netperf_windows_path

(F841)

🔇 Additional comments (1)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (1)

298-307: Teardown flow and VM handling look consistent with tp-libvirt conventions.

Teardown only cleans guest logs and remote tcpdump/netserver and leaves VM lifecycle to the framework, which matches other tp-libvirt tests where explicit VM destroy is avoided in test code. Based on learnings.

Comment on lines +233 to +245
netperf_install_path = params.get("netperf_install_path")
tcpdump_log_file = '/tmp/UDP_tcpdump.log'

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid fixed tcpdump log path in /tmp to prevent collisions between runs.

Using the constant /tmp/UDP_tcpdump.log means concurrent jobs or reruns on the same remote host can overwrite each other’s captures. You already use utils_misc.generate_random_string, so it’s easy to make this per‑test.

-    netperf_install_path = params.get("netperf_install_path")
-    tcpdump_log_file = '/tmp/UDP_tcpdump.log'
+    netperf_install_path = params.get("netperf_install_path")
+    tcpdump_log_file = os.path.join(
+        '/tmp', f'UDP_tcpdump_{vm_name}_{utils_misc.generate_random_string(4)}.log')

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Ruff (0.14.5)

234-234: Probable insecure usage of temporary file or directory: "/tmp/UDP_tcpdump.log"

(S108)

🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py
around lines 233 to 235, the tcpdump_log_file is hardcoded to
'/tmp/UDP_tcpdump.log' which can collide across concurrent runs; change it to
generate a unique path (e.g., use utils_misc.generate_random_string and
incorporate it into the filename or include the process id/timestamp) so each
test run writes to a distinct file in /tmp (ensure the generated string is
deterministic enough and still cleaned up after test).

@nanli1 nanli1 requested a review from cliping November 24, 2025 00:48
@nanli1
Copy link
Contributor Author

nanli1 commented Nov 24, 2025

@yanglei-rh

@nanli1 nanli1 force-pushed the add_case_for_local_VM_transfer_packets_to_remote_host_through_udp branch from 5940466 to 9d655e4 Compare November 25, 2025 13:08
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (2)

284-284: Remove the debug log that leaks guest_passwd.

This line logs the guest password which is a credential leak (already flagged in previous review). Please remove this debug line entirely:

-        test.log.debug("ssssssssssssssssssssssssss:%s", guest_passwd)
         if guest_os_type == 'linux':

244-244: Use unique tcpdump log path to avoid concurrent run collisions.

The hardcoded path /tmp/UDP_tcpdump.log can cause conflicts when multiple tests run concurrently (already flagged in previous review). Generate a unique filename:

-    tcpdump_log_file = '/tmp/UDP_tcpdump.log'
+    tcpdump_log_file = os.path.join(
+        '/tmp', f'UDP_tcpdump_{vm_name}_{utils_misc.generate_random_string(6)}.log')
🧹 Nitpick comments (1)
libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg (1)

22-22: Consider using modern firewall command for consistency.

The netsh firewall syntax is deprecated. While the Python test script (line 57) already includes the modern netsh advfirewall command, consider updating this config to match:

-        firewall_cmd = "netsh firewall set opmode mode=disable"
+        firewall_cmd = "netsh advfirewall set allprofiles state off"

This provides consistency between config and code, though it's not critical since the Python script handles both.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5940466 and 9d655e4.

⛔ Files ignored due to path filters (2)
  • libvirt/tests/deps/netperf/netperf-2.7.1.tar.bz2 is excluded by !**/*.bz2
  • libvirt/tests/deps/netperf/netperf.exe is excluded by !**/*.exe
📒 Files selected for processing (2)
  • libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg (1 hunks)
  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (1 hunks)
🧰 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.780Z
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.780Z
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.780Z
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/transfer_packets_to_remote_host_through_UDP.cfg
  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.

Applied to files:

  • libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg
  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py
📚 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/transfer_packets_to_remote_host_through_UDP.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_domfsfreeze_domfsthaw.py (1)
  • generate_random_string (104-113)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py

59-59: Do not catch blind exception: Exception

(BLE001)


217-217: Do not catch blind exception: Exception

(BLE001)


225-225: Do not catch blind exception: Exception

(BLE001)


238-238: Local variable local_passwd is assigned to but never used

Remove assignment to unused variable local_passwd

(F841)


244-244: Probable insecure usage of temporary file or directory: "/tmp/UDP_tcpdump.log"

(S108)


248-248: Local variable netperf_windows_path is assigned to but never used

Remove assignment to unused variable netperf_windows_path

(F841)

⏰ 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.11
  • GitHub Check: Python 3.8
  • GitHub Check: Python 3.12
🔇 Additional comments (10)
libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg (1)

1-20: LGTM! Configuration parameters are well-structured.

The test configuration appropriately defines VM lifecycle, network parameters, and netperf commands for Linux. The installation command correctly handles the complete build chain for netperf from source.

libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (9)

1-38: LGTM! Clean imports and well-documented function signature.

The imports are appropriate for the virttest framework, and the docstring clearly outlines the test workflow.


40-61: LGTM! Firewall handling appropriately handles both platforms.

The function correctly disables firewalls on Linux and Windows with appropriate error handling. The broad exception catch at line 59 is justified here since the firewall may not exist or already be disabled, and the exception is logged for debugging.


62-90: LGTM! Netperf transfer and installation logic is solid.

The function properly handles file transfer, conditional installation (Linux only), and verification with appropriate timeouts and error checking.


91-133: LGTM! Netserver setup handles dependencies and conflicts well.

The function properly installs netserver, handles port conflicts by killing existing processes, manages the libsctp dependency, and verifies successful startup.


134-152: LGTM! Netperf execution handles both OS types correctly.

The function appropriately runs netperf with OS-specific paths and generates unique log filenames to avoid collisions.


154-189: LGTM! Log validation properly waits for completion and verifies content.

The function uses appropriate wait mechanisms for both OS types and validates that the netperf log contains the expected packet size data.


190-228: LGTM! Packet verification includes helpful debug fallback.

The function properly searches for captured packets and provides useful diagnostic output if packets aren't found. The broad exception catches (lines 217, 225) are acceptable here as they're in the debug/diagnostic path and failures are logged before ultimately calling test.fail.


265-307: LGTM! Test workflow is well-structured and comprehensive.

The test properly orchestrates the complete workflow: VM startup, IP acquisition, netserver setup, tcpdump capture, netperf transfer/execution, and verification. The use of TEST_STEP logging makes the flow easy to follow. (Note: the password logging at line 284 must still be fixed per separate comment.)


308-323: LGTM! Cleanup and error handling follow framework best practices.

The teardown function properly cleans up logs and processes, and the try/finally block ensures cleanup runs regardless of test outcome. This is consistent with libvirt test framework patterns.

remote_ip = params.get("remote_ip")
remote_user = params.get("remote_user", "root")
remote_passwd = params.get("remote_pwd")
local_passwd = params.get("local_pwd")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Remove unused variables.

Static analysis correctly identifies that local_passwd (line 238) and netperf_windows_path (line 248) are assigned but never used in the code. Please remove them:

-    local_passwd = params.get("local_pwd")
     guest_passwd = params.get("password")
     netperf_linux_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)
-    netperf_windows_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)

Also applies to: 248-248

🧰 Tools
🪛 Ruff (0.14.5)

238-238: Local variable local_passwd is assigned to but never used

Remove assignment to unused variable local_passwd

(F841)

🤖 Prompt for AI Agents
In
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py
around lines 238 and 248 remove the unused variable assignments for local_passwd
and netperf_windows_path; delete the lines that assign local_passwd =
params.get("local_pwd") and netperf_windows_path =
params.get("netperf_windows_path") (or replace with a comment if you need to
document why they were considered) and run tests/static analysis to ensure no
other references exist.

  xxxx-300100:[virtual network][virtual-nic-device]The local VM can transfer length 1473-1480 packets to remote host through UDP
Signed-off-by: nanli <[email protected]>
@nanli1 nanli1 force-pushed the add_case_for_local_VM_transfer_packets_to_remote_host_through_udp branch from 9d655e4 to 8d128b4 Compare November 25, 2025 13:42
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (2)

238-248: Remove unused local_passwd and netperf_windows_path assignments.

local_passwd and netperf_windows_path are assigned but never used, which triggers Ruff F841 and adds noise:

-    remote_passwd = params.get("remote_pwd")
-    local_passwd = params.get("local_pwd")
+    remote_passwd = params.get("remote_pwd")
@@
-    netperf_pkg = params.get('netperf_pkg')
-    netperf_linux_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)
-    netperf_windows_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)
+    netperf_pkg = params.get('netperf_pkg')
+    netperf_linux_path = os.path.join(data_dir.get_deps_dir("netperf"), netperf_pkg)

If you plan to use them later, adding a short comment instead would clarify the intent.


244-244: Avoid fixed /tmp/UDP_tcpdump.log filename to prevent collisions across runs.

Using a constant path in /tmp means concurrent or back‑to‑back tests on the same remote host can overwrite each other’s tcpdump logs and trip the S108 warning.

You already depend on utils_misc.generate_random_string, so making the filename per‑test is straightforward:

-    tcpdump_log_file = '/tmp/UDP_tcpdump.log'
+    tcpdump_log_file = os.path.join(
+        '/tmp',
+        f'UDP_tcpdump_{vm_name}_{utils_misc.generate_random_string(4)}.log')

All subsequent uses (tcpdump start and teardown rm -f) will keep working via the variable.

🧹 Nitpick comments (3)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (3)

47-60: Guard against missing firewall_cmd and avoid blind Exception catch in disable_firewall.

If firewall_cmd is unset, session.cmd_status(firewall_cmd) / session.cmd(firewall_cmd, ...) will likely fail in a confusing way. Also the Windows branch swallows every Exception, which makes real problems (e.g. session issues) easy to miss.

You can make this more robust and explicit:

-        firewall_cmd = params.get("firewall_cmd")
-
-        if guest_os_type == "linux":
-            status = session.cmd_status(firewall_cmd)
+        firewall_cmd = params.get("firewall_cmd")
+        if not firewall_cmd:
+            test.log.debug("No firewall_cmd configured for %s guest, skip disabling firewall",
+                           guest_os_type)
+            return
+
+        if guest_os_type == "linux":
+            status = session.cmd_status(firewall_cmd)
@@
-        else:
-            try:
-                session.cmd(firewall_cmd, timeout=30)
+        else:
+            try:
+                session.cmd(firewall_cmd, timeout=30)
                 test.log.debug("Disabled legacy Windows firewall")
                 session.cmd('netsh advfirewall set allprofiles state off', timeout=30)
                 test.log.debug("Disabled Windows advanced firewall")
-            except Exception as e:
+            except Exception as e:  # consider narrowing to the concrete session error type
                 test.log.debug("Could not disable Windows firewall (may not exist or already disabled): %s", e)

179-188: Actually use data_match as a content check when validating the netperf log.

Currently the code only checks that data_match is truthy and that packet_size appears in the log; the actual value of data_match is ignored. If the cfg provides a specific string to match, that expectation is never enforced.

You can keep backward compatibility while honoring data_match like this:

-        output = guest_session.cmd_output(viewlog_cmd)
-        if data_match and str(packet_size) in output:
+        output = guest_session.cmd_output(viewlog_cmd)
+        # If data_match is configured, require it; otherwise only check packet_size.
+        if (not data_match or data_match in output) and str(packet_size) in output:
             test.log.debug('The log of netperf checking is PASS')
         else:
             test.fail("The log of netperf isn't right:%s" % output)

201-227: Tighten tcpdump grep regex and consider narrowing the exception type.

The grep pattern uses the raw IPs inside an extended regex, so . in the IP addresses act as wildcards. That’s harmless most of the time but can lead to unexpected matches. Also, the blanket except Exception hides the concrete failure mode.

You can escape the IPs and still keep the approach simple:

-        expected_pattern = r'IP %s\.[0-9]+ > %s\.[0-9]+: UDP, length %s' % (vm_ip, remote_ip, packet_size)
+        expected_pattern = r'IP %s\.[0-9]+ > %s\.[0-9]+: UDP, length %s' % (
+            re.escape(vm_ip), re.escape(remote_ip), packet_size)

And, if practical, narrow the exception to the specific session error type instead of bare Exception:

-        try:
-            grep_output = remote_session.cmd_output(grep_cmd).strip()
+        try:
+            grep_output = remote_session.cmd_output(grep_cmd).strip()
@@
-        except Exception as e:
+        except Exception as e:  # consider narrowing this to the concrete remote-session exception
             test.log.debug("Grep search failed: %s", str(e))
@@
-        except Exception as e:
+        except Exception as e:  # same here
             test.log.debug("Could not read log file: %s", str(e))
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d655e4 and 8d128b4.

⛔ Files ignored due to path filters (2)
  • libvirt/tests/deps/netperf/netperf-2.7.1.tar.bz2 is excluded by !**/*.bz2
  • libvirt/tests/deps/netperf/netperf.exe is excluded by !**/*.exe
📒 Files selected for processing (2)
  • libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.cfg (1 hunks)
  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libvirt/tests/cfg/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.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.780Z
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.780Z
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.780Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.

Applied to files:

  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.

Applied to files:

  • libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py
📚 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/transfer_packets_to_remote_host_through_UDP.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py (1)
provider/virtual_network/network_base.py (1)
  • get_vm_ip (22-57)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/transfer_packets_to_remote_host_through_UDP.py

59-59: Do not catch blind exception: Exception

(BLE001)


217-217: Do not catch blind exception: Exception

(BLE001)


225-225: Do not catch blind exception: Exception

(BLE001)


238-238: Local variable local_passwd is assigned to but never used

Remove assignment to unused variable local_passwd

(F841)


244-244: Probable insecure usage of temporary file or directory: "/tmp/UDP_tcpdump.log"

(S108)


248-248: Local variable netperf_windows_path is assigned to but never used

Remove assignment to unused variable netperf_windows_path

(F841)

⏰ 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.11
  • GitHub Check: Python 3.8

@nanli1 nanli1 removed the request for review from cliping November 26, 2025 05:24
@nanli1
Copy link
Contributor Author

nanli1 commented Nov 26, 2025

@yanglei-rh hi yanglei, please help check this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants