virtual_network: add case for basic QinQ check test #6574
Conversation
WalkthroughAdds a QinQ (VLAN stacking) end-to-end test to libvirt tests via three new files. A new config file defines parameters for a bridge-backed QinQ scenario and VM lifecycle controls. A Python test (libvirt/tests/src/virtual_network/qemu/bridge_qinq.py) orchestrates VM creation, dynamic guest-interface XML edits, host bridge and nested VLAN (802.1ad parent + 802.1q child) setup, tcpdump-based packet validation, host↔guest reachability and file-transfer with MD5 verification, and cleanup. A shell helper (libvirt/tests/deps/linux_qinq/qinq.sh) creates the nested VLAN hierarchy on a specified host interface. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-10-24T06:35:25.679ZApplied to files:
📚 Learning: 2025-09-24T08:01:27.899ZApplied to files:
📚 Learning: 2025-11-03T13:30:16.458ZApplied to files:
📚 Learning: 2025-11-18T08:47:14.465ZApplied to files:
🧬 Code graph analysis (1)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
🪛 Ruff (0.14.5)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py145-145: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file" (S108) 148-148: Use of possibly insecure function; consider using (S307) ⏰ 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)
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: 6
🧹 Nitpick comments (12)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg (2)
33-35: 4 GiB transfer by default is overkill for CI4096 (MiB) will bloat runtime and storage; ping/tcpdump already validate datapath.
Consider:
- file_size = 4096 + file_size = 64
20-21: Hard‑coded MAC may collideFixed MAC on a host iface can conflict with existing addresses or NIC policies.
Either omit advlan_mac and skip set_mac in code, or generate a locally-administered MAC at runtime.
provider/virtual_network/script/qinq.sh (4)
1-1: Harden the shell scriptEnable strict mode to catch errors early.
-#!/bin/sh +#!/bin/sh +set -euo pipefail
15-18: Cleanup only when both files existIf one exists, it’s left behind.
-if [ -e $file1 ] && [ -e $file2 ] -then - rm -f $file1 $file2 -fi +rm -f "$file1" "$file2"
20-40: Scale of VLAN creation is excessive for a “basic” testCreating 10×1024 subinterfaces inside the guest is heavy and slow; the Python test only uses v1v10 and v2v10_20.
Add optional args to limit creation (e.g., outer VLAN id and one inner id), or set much smaller defaults (vns_pre=1, vns=32) and let stress tests override.
41-49: Message accuracy and robustnessret2 counts 802.1Q subifaces; message says “dot1ad vlans”.
- echo "$ret2 dot1ad vlans created successfully" + echo "$ret2 VLAN subinterfaces created successfully"Also quote vars in tests/echo for safety.
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (6)
132-139: Prefer exception logging for debugging md5sum issuesPreserve traceback for IndexError.
- except IndexError: - test.log.error("Could not get file md5sum in guest") + except IndexError: + LOG.exception("Could not get file md5sum in guest")
301-307: Over‑permissive loss thresholdAllowing up to 4/10 packets to drop (>=5%) is too lax for a basic connectivity check.
- if package_lost >= 5: + if package_lost != 0: test.fail( - "%s packeage lost when ping guest ip %s " + "%s packet loss when pinging guest ip %s " % (package_lost, L2tag_iface_ip) )
325-330: MTU tweak may be insufficient for QinQ1504 covers single 802.1Q; QinQ typically needs 1508. This path only pings small packets, but future larger frames will fragment/drop.
Consider 1508 (or negotiated jumbo) when QinQ is active:
- session.cmd("ip link set %s mtu 1504" % nic_name) + session.cmd("ip link set %s mtu 1508" % nic_name)Confirm against your target qemu/NIC behavior.
225-236: Setting a fixed MAC on host VLAN iface is riskyMay conflict with existing MACs and surprises switches.
Avoid set_mac unless required by the environment; let kernel auto‑assign, or generate a locally‑administered MAC.
167-170: Large file creation can dominate runtimedd 64–256 MiB is sufficient to validate scp+md5.
Align default file_size with CI limits and make it overrideable via params.
Also applies to: 331-343
190-196: Guest IP assignment robustnessConsider adding “ip link set up” before assigning and validating with “ip addr show”; some guests require explicit up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py(1 hunks)provider/virtual_network/script/qinq.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.13.1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
137-137: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
161-161: Use of possibly insecure function; consider using ast.literal_eval
(S307)
168-168: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file"
(S108)
178-178: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.11
- GitHub Check: Python 3.12
| tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s" | ||
| tcpdump_log = "/tmp/tcpdump-%s.log" | ||
| get_tcpdump_log_cmd = "cat ${tcpdump_log}" | ||
| advlan_name = "${private_bridge}-vl${vlan_id}" | ||
| add_advlan_cmd = "ip link add link ${private_bridge} name ${advlan_name} type vlan proto 802.1ad id ${vlan_id}" |
There was a problem hiding this comment.
tcpdump not backgrounded → test may hang
The command lacks “&” (and stderr redirection). With session.sendline(), tcpdump likely occupies the shell and subsequent commands get fed to tcpdump stdin.
Apply:
- tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s"
+ tcpdump_cmd = "nohup tcpdump -xxvvleni %s > %s 2>&1 &"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s" | |
| tcpdump_log = "/tmp/tcpdump-%s.log" | |
| get_tcpdump_log_cmd = "cat ${tcpdump_log}" | |
| advlan_name = "${private_bridge}-vl${vlan_id}" | |
| add_advlan_cmd = "ip link add link ${private_bridge} name ${advlan_name} type vlan proto 802.1ad id ${vlan_id}" | |
| tcpdump_cmd = "nohup tcpdump -xxvvleni %s > %s 2>&1 &" | |
| tcpdump_log = "/tmp/tcpdump-%s.log" | |
| get_tcpdump_log_cmd = "cat ${tcpdump_log}" | |
| advlan_name = "${private_bridge}-vl${vlan_id}" | |
| add_advlan_cmd = "ip link add link ${private_bridge} name ${advlan_name} type vlan proto 802.1ad id ${vlan_id}" |
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg around lines 15 to 19,
the tcpdump command is run in the foreground which can block the test shell;
update tcpdump_cmd so tcpdump is backgrounded and its stderr is captured — e.g.
append "2>&1 &" (or use nohup/setsid with trailing "&") so the process detaches
and subsequent commands run; ensure the redirected output still points to the
tcpdump_log variable.
There was a problem hiding this comment.
TO AI: setsid could also run a cmd in background.
There was a problem hiding this comment.
@qiankehan, you're absolutely right that setsid is useful for detaching from the controlling terminal. However, setsid alone doesn't background the process—you still need the & operator at the end. The original command:
setsid tcpdump -xxvvleni %s > %s
will still block the shell even with setsid, because it runs in the foreground. To properly background it while keeping setsid, the command should be:
setsid tcpdump -xxvvleni %s > %s 2>&1 &
This adds:
2>&1to capture stderr along with stdout&to actually run the process in the background
So if you prefer to keep setsid (which is fine for session isolation), just make sure to add those two missing parts.
| def check_tcpdump_result( | ||
| session, | ||
| iface_name, | ||
| ethertype, | ||
| ethertype2=None, | ||
| vlan_tag=None, | ||
| vlan_tag2=None, | ||
| enable_logging=False, | ||
| ): | ||
| """ | ||
| Check tcpdump result. | ||
|
|
||
| :param session: guest session | ||
| :param iface_name: the tcpdump file of the interface | ||
| :param ethertype: ethertype value need to be matched | ||
| :param ethertype2: ethertype value 2 needed to be matched if not None | ||
| :param vlan_tag: vlan tag value needed to be matched if not None | ||
| :param vlan_tag2: vlan tag value 2 needed to be matched if not None | ||
| :param enable_logging: whether to dump tcpdump results during test | ||
| """ | ||
| get_tcpdump_log_cmd = params["get_tcpdump_log_cmd"] % iface_name | ||
| tcpdump_content = session.cmd_output( | ||
| get_tcpdump_log_cmd, timeout=300, safe=True | ||
| ).strip() | ||
| lines = tcpdump_content.splitlines() | ||
| sum = 0 | ||
| for i in range(len(lines)): | ||
| if enable_logging: | ||
| test.log.info("line %s: %s", i, lines[i]) | ||
| if not ethertype2: | ||
| if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: | ||
| sum += 1 | ||
| if vlan_tag and vlan_tag not in lines[i - 1]: | ||
| if "too much work for irq" in lines[i - 1]: | ||
| continue | ||
| else: | ||
| test.fail( | ||
| "in %s tcpdump log, there should be vlan " | ||
| "tag %s" % (iface_name, vlan_tag) | ||
| ) | ||
| elif not vlan_tag: | ||
| if "vlan" in lines[i - 1]: | ||
| test.fail( | ||
| "in %s tcpdump log, there should not be " | ||
| "vlan tag" % iface_name | ||
| ) | ||
| else: | ||
| if ( | ||
| "ICMP echo re" in lines[i] | ||
| and ethertype in lines[i - 1] | ||
| and ethertype2 in lines[i - 1] | ||
| ): | ||
| sum += 1 | ||
| if vlan_tag not in lines[i - 1] or vlan_tag2 not in lines[i - 1]: | ||
| if "too much work for irq" in lines[i - 1]: | ||
| continue | ||
| else: | ||
| test.fail( | ||
| "in %s tcpdump log, there should be vlan " | ||
| "tag %s" % (iface_name, vlan_tag) | ||
| ) | ||
| if sum == 0: | ||
| test.fail( | ||
| "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) | ||
| ) | ||
|
|
There was a problem hiding this comment.
tcpdump parsing off‑by‑one; shadowing built‑in sum
Loop starts at 0 but indexes i-1; at i=0 this reads the last line. Also shadows built‑in sum().
- lines = tcpdump_content.splitlines()
- sum = 0
- for i in range(len(lines)):
+ lines = tcpdump_content.splitlines()
+ matches = 0
+ for i in range(1, len(lines)):
if enable_logging:
test.log.info("line %s: %s", i, lines[i])
if not ethertype2:
if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]:
- sum += 1
+ matches += 1
@@
- if sum == 0:
+ if matches == 0:
test.fail(
"in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype)
)📝 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 check_tcpdump_result( | |
| session, | |
| iface_name, | |
| ethertype, | |
| ethertype2=None, | |
| vlan_tag=None, | |
| vlan_tag2=None, | |
| enable_logging=False, | |
| ): | |
| """ | |
| Check tcpdump result. | |
| :param session: guest session | |
| :param iface_name: the tcpdump file of the interface | |
| :param ethertype: ethertype value need to be matched | |
| :param ethertype2: ethertype value 2 needed to be matched if not None | |
| :param vlan_tag: vlan tag value needed to be matched if not None | |
| :param vlan_tag2: vlan tag value 2 needed to be matched if not None | |
| :param enable_logging: whether to dump tcpdump results during test | |
| """ | |
| get_tcpdump_log_cmd = params["get_tcpdump_log_cmd"] % iface_name | |
| tcpdump_content = session.cmd_output( | |
| get_tcpdump_log_cmd, timeout=300, safe=True | |
| ).strip() | |
| lines = tcpdump_content.splitlines() | |
| sum = 0 | |
| for i in range(len(lines)): | |
| if enable_logging: | |
| test.log.info("line %s: %s", i, lines[i]) | |
| if not ethertype2: | |
| if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: | |
| sum += 1 | |
| if vlan_tag and vlan_tag not in lines[i - 1]: | |
| if "too much work for irq" in lines[i - 1]: | |
| continue | |
| else: | |
| test.fail( | |
| "in %s tcpdump log, there should be vlan " | |
| "tag %s" % (iface_name, vlan_tag) | |
| ) | |
| elif not vlan_tag: | |
| if "vlan" in lines[i - 1]: | |
| test.fail( | |
| "in %s tcpdump log, there should not be " | |
| "vlan tag" % iface_name | |
| ) | |
| else: | |
| if ( | |
| "ICMP echo re" in lines[i] | |
| and ethertype in lines[i - 1] | |
| and ethertype2 in lines[i - 1] | |
| ): | |
| sum += 1 | |
| if vlan_tag not in lines[i - 1] or vlan_tag2 not in lines[i - 1]: | |
| if "too much work for irq" in lines[i - 1]: | |
| continue | |
| else: | |
| test.fail( | |
| "in %s tcpdump log, there should be vlan " | |
| "tag %s" % (iface_name, vlan_tag) | |
| ) | |
| if sum == 0: | |
| test.fail( | |
| "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) | |
| ) | |
| def check_tcpdump_result( | |
| session, | |
| iface_name, | |
| ethertype, | |
| ethertype2=None, | |
| vlan_tag=None, | |
| vlan_tag2=None, | |
| enable_logging=False, | |
| ): | |
| """ | |
| Check tcpdump result. | |
| :param session: guest session | |
| :param iface_name: the tcpdump file of the interface | |
| :param ethertype: ethertype value need to be matched | |
| :param ethertype2: ethertype value 2 needed to be matched if not None | |
| :param vlan_tag: vlan tag value needed to be matched if not None | |
| :param vlan_tag2: vlan tag value 2 needed to be matched if not None | |
| :param enable_logging: whether to dump tcpdump results during test | |
| """ | |
| get_tcpdump_log_cmd = params["get_tcpdump_log_cmd"] % iface_name | |
| tcpdump_content = session.cmd_output( | |
| get_tcpdump_log_cmd, timeout=300, safe=True | |
| ).strip() | |
| lines = tcpdump_content.splitlines() | |
| matches = 0 | |
| for i in range(1, len(lines)): | |
| if enable_logging: | |
| test.log.info("line %s: %s", i, lines[i]) | |
| if not ethertype2: | |
| if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: | |
| matches += 1 | |
| if vlan_tag and vlan_tag not in lines[i - 1]: | |
| if "too much work for irq" in lines[i - 1]: | |
| continue | |
| else: | |
| test.fail( | |
| "in %s tcpdump log, there should be vlan " | |
| "tag %s" % (iface_name, vlan_tag) | |
| ) | |
| elif not vlan_tag: | |
| if "vlan" in lines[i - 1]: | |
| test.fail( | |
| "in %s tcpdump log, there should not be " | |
| "vlan tag" % iface_name | |
| ) | |
| else: | |
| if ( | |
| "ICMP echo re" in lines[i] | |
| and ethertype in lines[i - 1] | |
| and ethertype2 in lines[i - 1] | |
| ): | |
| matches += 1 | |
| if vlan_tag not in lines[i - 1] or vlan_tag2 not in lines[i - 1]: | |
| if "too much work for irq" in lines[i - 1]: | |
| continue | |
| else: | |
| test.fail( | |
| "in %s tcpdump log, there should be vlan " | |
| "tag %s" % (iface_name, vlan_tag) | |
| ) | |
| if matches == 0: | |
| test.fail( | |
| "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) | |
| ) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_qinq.py around lines 58 to 123,
the tcpdump parsing loop starts at i=0 but accesses lines[i-1] (causing an
unintended wrap to the last line) and uses the name "sum" which shadows the
built-in; change the loop to start from 1 (e.g., for i in range(1, len(lines)))
or iterate with enumerate and keep track of the previous line explicitly, and
rename the "sum" variable to a non-built-in name like "match_count" (or "count")
throughout; ensure any early-continue/conditions still reference the correct
previous line and update the final check to use the new variable name.
| network_dict = eval(params.get("network_dict")) | ||
| net_dev = network_xml.NetworkXML() | ||
| net_dev.setup_attrs(**network_dict) | ||
|
|
There was a problem hiding this comment.
Avoid eval on untrusted params
Use ast.literal_eval.
+import ast
@@
- network_dict = eval(params.get("network_dict"))
+ network_dict = ast.literal_eval(params.get("network_dict"))
@@
- iface_dict = eval(params.get("iface_dict"))
+ iface_dict = ast.literal_eval(params.get("iface_dict"))Also applies to: 178-181
🧰 Tools
🪛 Ruff (0.13.1)
161-161: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_qinq.py around lines 161-164
and 178-181, the code calls eval(params.get("network_dict")) on test input which
is unsafe; replace eval with ast.literal_eval to safely parse the string into a
Python literal, add "import ast" at the top if missing, and guard against
None/invalid input by checking params.get("network_dict") is not None and
catching ValueError/SyntaxError to raise a clear error or fallback.
| # Start tcpdump on L1tag interface and first_nic in guest | ||
| test.log.info("Start tcpdump in %s", vm_name) | ||
| L1tag_tcpdump_log = params.get("tcpdump_log") % L1tag_iface | ||
| L1tag_tcpdump_cmd = params.get("tcpdump_cmd") % (L1tag_iface, L1tag_tcpdump_log) | ||
| first_nic_tcpdump_log = params.get("tcpdump_log") % nic_name | ||
| first_nic_tcpdump_cmd = params.get("tcpdump_cmd") % ( | ||
| nic_name, | ||
| first_nic_tcpdump_log, | ||
| ) | ||
| session.sendline(L1tag_tcpdump_cmd) | ||
| time.sleep(2) | ||
| session.sendline(first_nic_tcpdump_cmd) | ||
| time.sleep(5) | ||
|
|
There was a problem hiding this comment.
tcpdump not backgrounded (paired with cfg); sendline will block the shell
Without “&”, sendline likely leaves the shell busy under tcpdump, making subsequent sendlines unreliable.
After fixing tcpdump_cmd in cfg, keep a short wait and optionally verify the process is running, or append “; echo READY” and wait for prompt.
| session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) | ||
| virsh.destroy(vm_name) | ||
|
|
||
| # Cleanup network | ||
| virsh.net_destroy(network_name, ignore_status=True) | ||
| virsh.net_undefine(network_name, ignore_status=True) | ||
|
|
||
| # Cleanup host interfaces | ||
| process.run(f"ip link del {advlan_ifname}", ignore_status=True) | ||
| process.run(f"ip link del {qvlan_ifname}", ignore_status=True) | ||
|
|
There was a problem hiding this comment.
UnboundLocal on cleanup if setup fails early
advlan_ifname/qvlan_ifname are defined in try; finally may raise before restore.
- try:
+ advlan_ifname = None
+ qvlan_ifname = None
+ try:
@@
- process.run(f"ip link del {advlan_ifname}", ignore_status=True)
- process.run(f"ip link del {qvlan_ifname}", ignore_status=True)
+ if advlan_ifname:
+ process.run(f"ip link del {advlan_ifname}", ignore_status=True)
+ if qvlan_ifname:
+ process.run(f"ip link del {qvlan_ifname}", ignore_status=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) | |
| virsh.destroy(vm_name) | |
| # Cleanup network | |
| virsh.net_destroy(network_name, ignore_status=True) | |
| virsh.net_undefine(network_name, ignore_status=True) | |
| # Cleanup host interfaces | |
| process.run(f"ip link del {advlan_ifname}", ignore_status=True) | |
| process.run(f"ip link del {qvlan_ifname}", ignore_status=True) | |
| advlan_ifname = None | |
| qvlan_ifname = None | |
| try: | |
| session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) | |
| virsh.destroy(vm_name) | |
| # Cleanup network | |
| virsh.net_destroy(network_name, ignore_status=True) | |
| virsh.net_undefine(network_name, ignore_status=True) | |
| # Cleanup host interfaces | |
| if advlan_ifname: | |
| process.run(f"ip link del {advlan_ifname}", ignore_status=True) | |
| if qvlan_ifname: | |
| process.run(f"ip link del {qvlan_ifname}", ignore_status=True) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_qinq.py around lines 345 to
355, advlan_ifname and qvlan_ifname are created inside a try block and the
finally cleanup unconditionally uses them, which can raise UnboundLocalError if
setup fails early; initialize both variables to None (or empty strings) before
the try and in the finally guard calls to process.run with a truthy check (e.g.
if advlan_ifname: process.run(...)) so cleanup only runs when the names were
actually set.
nanli1
force-pushed
the
add_case_for_basic_QinQ_check_test_checking
branch
4 times, most recently
from
fb02151 to
b40b1bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (4)
217-220: Ensure tcpdump processes are backgrounded and running.After fixing
tcpdump_cmdin the config file to include&, verify the processes are running before continuing.Consider adding a verification step after the sleeps:
session.sendline(L1tag_tcpdump_cmd) time.sleep(2) session.sendline(first_nic_tcpdump_cmd) time.sleep(5) # Verify tcpdump processes are running output = session.cmd_output("pgrep -f tcpdump") if not output.strip(): test.error("tcpdump processes failed to start")
77-114: Fix off-by-one error and rename variable shadowing built-in.The loop starts at index 0 but accesses
lines[i - 1], which wraps to the last line wheni=0. The variable namesumshadows Python's built-in.Apply this diff:
lines = tcpdump_content.splitlines() - sum = 0 - for i in range(len(lines)): + match_count = 0 + for i in range(1, len(lines)): if enable_logging: test.log.info("line %s: %s", i, lines[i]) if not ethertype2: if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: - sum += 1 + match_count += 1 if vlan_tag and vlan_tag not in lines[i - 1]: if "too much work for irq" in lines[i - 1]: continue else: test.fail( "in %s tcpdump log, there should be vlan " "tag %s" % (iface_name, vlan_tag) ) elif not vlan_tag: if "vlan" in lines[i - 1]: test.fail( "in %s tcpdump log, there should not be " "vlan tag" % iface_name ) else: if ( "ICMP echo re" in lines[i] and ethertype in lines[i - 1] and ethertype2 in lines[i - 1] ): - sum += 1 + match_count += 1 if vlan_tag not in lines[i - 1] or vlan_tag2 not in lines[i - 1]: if "too much work for irq" in lines[i - 1]: continue else: test.fail( "in %s tcpdump log, there should be vlan " "tag %s" % (iface_name, vlan_tag) ) - if sum == 0: + if match_count == 0: test.fail( "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) ) - test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", - iface_name, sum, ethertype, + test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", + iface_name, match_count, ethertype, f" and {ethertype2}" if ethertype2 else "", f" with vlan tags {vlan_tag}{vlan_tag2 if vlan_tag2 else ''}" if vlan_tag else "")
154-154: Replace eval() with ast.literal_eval for security.Using
eval()on test parameters is unsafe and can execute arbitrary code.Apply this diff:
+import ast + @@ - iface_dict = eval(params.get("iface_dict")) + iface_dict = ast.literal_eval(params.get("iface_dict"))
340-348: Initialize cleanup variables and guard cleanup calls.Multiple variables used in the
finallyblock may not be defined if setup fails early:session,advlan_ifname, andqvlan_ifname.Apply this diff:
+ session = None + advlan_ifname = None + qvlan_ifname = None try: @@ finally: utils_net.delete_linux_bridge_tmux(bridge_name, host_iface) - session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) + if session: + session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) virsh.destroy(vm_name) # Cleanup host interfaces - process.run(f"ip link del {advlan_ifname}", ignore_status=True) - process.run(f"ip link del {qvlan_ifname}", ignore_status=True) + if advlan_ifname: + process.run(f"ip link del {advlan_ifname}", ignore_status=True) + if qvlan_ifname: + process.run(f"ip link del {qvlan_ifname}", ignore_status=True)libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg (1)
15-15: tcpdump must be backgrounded to prevent shell blocking.The command lacks
&and stderr redirection. When used withsession.sendline()in the test (lines 217, 219, 271-275 in bridge_qinq.py), tcpdump will occupy the foreground and subsequent commands will fail.Apply this diff:
- tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s" + tcpdump_cmd = "nohup tcpdump -xxvvleni %s > %s 2>&1 &"provider/virtual_network/script/qinq.sh (1)
8-11: Quote variables and exit with non-zero status on error.Unquoted
$ifacecan cause word-splitting issues, andexit 0masks the error from calling scripts.Apply this diff:
-if [ -z $iface ] +if [ -z "${iface:-}" ] then - echo "please provide a interface, e.g. `basename $0` eth0" - exit 0 + echo "please provide an interface, e.g. $(basename "$0") eth0" >&2 + exit 1
🧹 Nitpick comments (8)
provider/virtual_network/script/qinq.sh (4)
15-18: Quote variable expansions in file tests.Unquoted variables can cause unexpected behavior if filenames contain spaces or special characters.
Apply this diff:
-if [ -e $file1 ] && [ -e $file2 ] +if [ -e "$file1" ] && [ -e "$file2" ] then - rm -f $file1 $file2 + rm -f "$file1" "$file2"
20-29: Quote variables and reconsider sleep placement.Multiple issues in the outer loop:
- Unquoted variables can cause word-splitting
sleep 2inside the outer loop adds 20 seconds total; if this is for interface stabilization, consider moving it after each VLAN or reducing the durationApply this diff to quote variables:
-for i in `seq $vns_pre` +for i in $(seq "$vns_pre") do - ip link add link $iface name v1v$i type vlan proto $proto id $i + ip link add link "$iface" name "v1v$i" type vlan proto "$proto" id "$i" if [ $? -eq 0 ] then - echo $i>>$file1 + echo "$i" >> "$file1" else - echo "v1v$i is not created" + echo "v1v$i is not created" >&2 fiConsider whether
sleep 2is necessary for each outer VLAN, or if it could be reduced/removed.
30-39: Quote variables in inner loop.Same quoting issues as the outer loop.
Apply this diff:
- for s in `seq $vns` + for s in $(seq "$vns") do - ip link add link v1v$i name v2v$i\_$s type vlan id $s + ip link add link "v1v$i" name "v2v${i}_${s}" type vlan id "$s" if [ $? -eq 0 ] then - echo $i\_$s>>$file2 + echo "${i}_${s}" >> "$file2" else - echo "v2v$i\_$s is not created" + echo "v2v${i}_${s} is not created" >&2 fi
41-43: Avoid useless use of cat and quote variables.The
cat | wc -lpattern is unnecessary;wc -l < fileis more efficient.Apply this diff:
-ret1=`cat $file1 |wc -l` -ret2=`cat $file2 |wc -l` -if [ $ret1 -eq $vns_pre ] && [ $ret2 -eq $((vns_pre*vns)) ] +ret1=$(wc -l < "$file1") +ret2=$(wc -l < "$file2") +if [ "$ret1" -eq "$vns_pre" ] && [ "$ret2" -eq $((vns_pre * vns)) ]libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (4)
177-177: Remove commented-out code.Commented code adds clutter and should be removed. If needed later, it can be retrieved from version control.
Apply this diff:
stop_NM_cmd = params.get("stop_NM_cmd") session.cmd(stop_NM_cmd, ignore_all_errors=True) - #session = vm.wait_for_login(timeout=login_timeout) test.log.info("TEST_STEP1:Copy qinq script to guest")
185-187: Remove commented-out code.These lines appear to be debugging artifacts.
Apply this diff:
remote.copy_files_to(vm_ip, 'scp', username, password, '22', host_qinq_dir, guest_qinq_dir) - - #vm.copy_files_to(host_qinq_dir, guest_qinq_dir) - - # vm.destroy(gracefully=True) nic_name = utils_net.get_linux_ifname(session, mac)
246-250: Document rationale for different packet loss thresholds.L1 VLAN test requires 0% packet loss (line 246) while L2 VLAN test allows up to 5% loss (line 300). If this difference is intentional due to nested VLAN overhead, consider adding a comment explaining the rationale.
132-137: Consider using test.log.exception for better diagnostics.When catching exceptions,
test.log.exception()automatically includes the traceback, which aids debugging.Apply this diff:
try: output = session.cmd_output("md5sum %s" % guest_path, 120).split()[0] guest_result = re.findall(r"\w+", output)[0] except IndexError: - test.log.error("Could not get file md5sum in guest") + test.log.exception("Could not get file md5sum in guest") return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py(1 hunks)provider/virtual_network/script/qinq.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/bridge_qinq.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
provider/virtual_network/network_base.py (1)
get_vm_ip(25-60)
🪛 Ruff (0.14.2)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
136-136: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
154-154: Use of possibly insecure function; consider using ast.literal_eval
(S307)
161-161: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file"
(S108)
nanli1
force-pushed
the
add_case_for_basic_QinQ_check_test_checking
branch
from
b40b1bf to
62d8d29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
provider/virtual_network/script/qinq.sh (1)
8-14: Make the interface-argument guard safe and fail correctly.The current test leaves
$ifaceunquoted and exits 0 on missing input, so the script silently succeeds when it should abort. Quote the variable, send the usage message to stderr, and return a non-zero status so the caller can detect the error.-if [ -z $iface ] -then - echo "please provide a interface, e.g. `basename $0` eth0" - exit 0 -else - echo "you are testing with $iface." -fi +if [ -z "${iface:-}" ]; then + echo "please provide an interface, e.g. $(basename "$0") eth0" >&2 + exit 1 +else + echo "you are testing with $iface." +filibvirt/tests/src/virtual_network/qemu/bridge_qinq.py (4)
78-121: Fix tcpdump parsing loop off-by-one.Iterating from 0 while reading
lines[i-1]means the first iteration inspects the last line, so we can report false matches or miss expected tags. It also shadows the built-insum(). Start the loop at 1 (or iterate overlines[1:]) and use a non-built-in counter.- lines = tcpdump_content.splitlines() - sum = 0 - for i in range(len(lines)): + lines = tcpdump_content.splitlines() + match_count = 0 + for i in range(1, len(lines)): if enable_logging: test.log.info("line %s: %s", i, lines[i]) if not ethertype2: if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: - sum += 1 + match_count += 1 @@ else: if ( "ICMP echo re" in lines[i] and ethertype in lines[i - 1] and ethertype2 in lines[i - 1] ): - sum += 1 + match_count += 1 @@ - if sum == 0: + if match_count == 0: test.fail( "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) ) - test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", - iface_name, sum, ethertype, - f" and {ethertype2}" if ethertype2 else "", - f" with vlan tags {vlan_tag}{vlan_tag2 if vlan_tag2 else ''}" if vlan_tag else "") + test.log.info( + "tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", + iface_name, + match_count, + ethertype, + f" and {ethertype2}" if ethertype2 else "", + f" with vlan tags {vlan_tag}{vlan_tag2 if vlan_tag2 else ''}" if vlan_tag else "", + )
213-216: Run tcpdump in the background before issuing follow-up commands.Each
session.sendlinelaunches tcpdump in the foreground, so the shell never returns to accept the next command; later sendlines just get buffered and the setup hangs intermittently. Background the tcpdump processes (or append the&in the command) and wait briefly for them to come up.- session.sendline(L1tag_tcpdump_cmd) + session.sendline(f"{L1tag_tcpdump_cmd} &") time.sleep(2) - session.sendline(first_nic_tcpdump_cmd) + session.sendline(f"{first_nic_tcpdump_cmd} &") @@ - session.sendline(L1tag_tcpdump_cmd) + session.sendline(f"{L1tag_tcpdump_cmd} &") time.sleep(2) - session.sendline(L2tag_tcpdump_cmd) + session.sendline(f"{L2tag_tcpdump_cmd} &") time.sleep(2) - session.sendline(first_nic_tcpdump_cmd) + session.sendline(f"{first_nic_tcpdump_cmd} &")Also applies to: 265-272
164-170: Make cleanup resilient when setup fails early.If an exception is raised before
session,advlan_ifname, orqvlan_ifnameare assigned, thefinallyblock raisesUnboundLocalErrorand leaks host resources. Initialise these names before thetry, guard their use, and letvirsh.destroyignore the “domain not running” case.- try: + session = None + advlan_ifname = None + qvlan_ifname = None + try: @@ - utils_net.delete_linux_bridge_tmux(bridge_name, host_iface) - session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) - virsh.destroy(vm_name) - - # Cleanup host interfaces - process.run(f"ip link del {advlan_ifname}", ignore_status=True) - process.run(f"ip link del {qvlan_ifname}", ignore_status=True) + utils_net.delete_linux_bridge_tmux(bridge_name, host_iface) + if session: + session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) + virsh.destroy(vm_name, ignore_status=True) + + # Cleanup host interfaces + if advlan_ifname: + process.run(f"ip link del {advlan_ifname}", ignore_status=True) + if qvlan_ifname: + process.run(f"ip link del {qvlan_ifname}", ignore_status=True)Also applies to: 336-343
154-155: Stop executing params with eval.
eval(params.get("iface_dict"))runs arbitrary code from the test configuration. Useast.literal_eval(and guard against missing/invalid input) instead, and make the same change for any other structured params.-import os +import ast +import os @@ - iface_dict = eval(params.get("iface_dict")) + iface_dict_raw = params.get("iface_dict") + if not iface_dict_raw: + test.cancel("iface_dict parameter is missing") + try: + iface_dict = ast.literal_eval(iface_dict_raw) + except (ValueError, SyntaxError) as exc: + test.fail(f"iface_dict parameter is not a valid literal: {exc}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py(1 hunks)provider/virtual_network/script/qinq.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg
🧰 Additional context used
🧠 Learnings (2)
📚 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/bridge_qinq.py
📚 Learning: 2025-11-03T13:30:16.458Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6531
File: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py:64-67
Timestamp: 2025-11-03T13:30:16.458Z
Learning: In tp-libvirt tests, prefer using a list of cleanup lambda functions that are appended when actions requiring cleanup are performed, then executed in a finally block with individual error handling. Example: `cleanup_actions.append(lambda: undo_action())` followed by iterating through the list in reverse order in the finally block.
Applied to files:
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
provider/virtual_network/network_base.py (1)
get_vm_ip(25-60)
🪛 Ruff (0.14.3)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
136-136: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
154-154: Use of possibly insecure function; consider using ast.literal_eval
(S307)
161-161: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file"
(S108)
⏰ 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.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
nanli1
force-pushed
the
add_case_for_basic_QinQ_check_test_checking
branch
from
62d8d29 to
3749576
Compare
nanli1
force-pushed
the
add_case_for_basic_QinQ_check_test_checking
branch
from
3749576 to
88e2138
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
232-238: Missing cleanup for created VLAN interfaces.The test creates
advlan_ifname(line 232) andqvlan_ifname(line 288) but the finally block (lines 347-353) doesn't clean them up. If the test fails or times out after creating these interfaces, they'll be left orphaned on the host.Based on learnings, consider adding cleanup actions to a list:
def run(test, params, env): # ... existing code ... cleanup_actions = [] try: # ... existing setup code ... # Line 234: After advlan creation process.run(add_advlan_cmd) cleanup_actions.append(lambda: process.run(f"ip link del {advlan_ifname}", ignore_status=True)) # Line 288: After qvlan creation process.system_output(add_qvlan_cmd) cleanup_actions.append(lambda: process.run(f"ip link del {qvlan_ifname}", ignore_status=True)) # ... rest of test ... finally: # Execute cleanup actions in reverse order for cleanup_action in reversed(cleanup_actions): try: cleanup_action() except Exception as e: test.log.warning(f"Cleanup failed: {e}") # ... existing finally cleanup ...As per coding guidelines.
Also applies to: 288-293
🧹 Nitpick comments (2)
libvirt/tests/deps/linux_qinq/qinq.sh (2)
22-22: Quote the interface variable and remove unnecessary backslashes.The unquoted
$ifaceon line 22 (and line 32's context) risks word-splitting if the interface name contains spaces or special characters. Additionally, the backslash before underscore inv2v$i\_$s(lines 32, 35) is unnecessary and reduces readability—shell variable syntax doesn't require escaping underscores.Apply this diff:
- ip link add link $iface name v1v$i type vlan proto $proto id $i + ip link add link "$iface" name v1v$i type vlan proto $proto id $i- ip link add link v1v$i name v2v$i\_$s type vlan id $s + ip link add link v1v$i name v2v${i}_${s} type vlan id $s- echo $i\_$s>>$file2 + echo ${i}_${s} >> "$file2"Also consider quoting
$file1and$file2consistently throughout.Also applies to: 32-32
41-42: Quote file paths and avoid uselesscat.The unquoted file paths and
cat | wc -lpattern are minor inefficiencies. While/tmp/pass1and/tmp/pass2are unlikely to contain spaces, defensive quoting is safer.Apply this diff:
-ret1=`cat $file1 |wc -l` -ret2=`cat $file2 |wc -l` +ret1=$(wc -l < "$file1") +ret2=$(wc -l < "$file2")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg(1 hunks)libvirt/tests/deps/linux_qinq/qinq.sh(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common 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.
📚 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/bridge_qinq.py
📚 Learning: 2025-11-03T13:30:16.458Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6531
File: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py:64-67
Timestamp: 2025-11-03T13:30:16.458Z
Learning: In tp-libvirt tests, prefer using a list of cleanup lambda functions that are appended when actions requiring cleanup are performed, then executed in a finally block with individual error handling. Example: `cleanup_actions.append(lambda: undo_action())` followed by iterating through the list in reverse order in the finally block.
Applied to files:
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
🪛 Ruff (0.14.4)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
136-136: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
157-157: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file"
(S108)
160-160: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
| finally: | ||
| session.cmd("rm -rf %s" % guest_path) | ||
| session.close() | ||
| virsh.destroy(vm_name) | ||
| host_bridge_iface.down() | ||
| host_bridges.del_bridge(brname) | ||
|
|
There was a problem hiding this comment.
Guard cleanup actions against uninitialized variables.
The finally block accesses session, host_bridge_iface, and host_bridges without checking if they were successfully initialized. If the test fails early (e.g., session creation fails at line 165 or 191, or bridge creation fails at line 180), the cleanup will raise NameError or AttributeError, masking the original failure.
Apply this diff:
+ session = None
+ host_bridge_iface = None
+ host_bridges = None
+
try:
vm = env.get_vm(params["main_vm"])
# ... rest of try block ...
finally:
- session.cmd("rm -rf %s" % guest_path)
- session.close()
+ if session:
+ try:
+ session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True)
+ session.close()
+ except Exception as e:
+ test.log.warning(f"Session cleanup failed: {e}")
+
virsh.destroy(vm_name)
- host_bridge_iface.down()
- host_bridges.del_bridge(brname)
+
+ if host_bridge_iface:
+ try:
+ host_bridge_iface.down()
+ except Exception as e:
+ test.log.warning(f"Bridge interface cleanup failed: {e}")
+
+ if host_bridges and brname:
+ try:
+ host_bridges.del_bridge(brname)
+ except Exception as e:
+ test.log.warning(f"Bridge deletion failed: {e}")Note: Based on learnings, VM cleanup is handled by the test framework's teardown, so virsh.destroy(vm_name) may be redundant but doesn't hurt.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/bridge_qinq.py around lines 347 to
353, the finally block assumes variables session, host_bridge_iface, and
host_bridges are always initialized which can raise NameError/AttributeError and
hide the original failure; update the cleanup to defensively check each resource
before using it (e.g., test for existence or not None via 'if "session" in
locals() and session is not None' or similar checks), wrap individual cleanup
steps in small try/except blocks to log but not raise on cleanup errors, only
call session.close() and session.cmd(...) if session was created, only call
host_bridge_iface.down() and host_bridges.del_bridge(brname) if those objects
exist, and keep virsh.destroy(vm_name) if desired (it's optional since teardown
handles VM cleanup).
88e2138 to
4a80859
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg (2)
8-8: Remove duplicateimage_snapshotsetting.The parameter
image_snapshotis defined twice with conflicting values ("yes" on line 8,noon line 33). The later assignment on line 33 will override line 8, making line 8 redundant and confusing.Apply this diff to remove the duplicate:
func_supported_since_qemu_kvm_ver = (8, 1, 0) create_vm_libvirt = "yes" - image_snapshot = "yes" kill_vm_libvirt = "yes"
20-20: Background tcpdump to prevent shell blocking.The tcpdump command lacks "&" for backgrounding and stderr redirection. When executed via
session.sendline()in the Python test, tcpdump will occupy the shell and subsequent commands may be fed to tcpdump's stdin or hang.Apply this diff:
- tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s" + tcpdump_cmd = "nohup tcpdump -xxvvleni %s > %s 2>&1 &"libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (4)
76-113: Fix off-by-one error and avoid shadowing built-insum.The loop starts at index 0 but accesses
lines[i - 1], which wraps to the last line wheni=0. Additionally, the variable namesumshadows Python's built-in function.Apply this diff:
lines = tcpdump_content.splitlines() - sum = 0 - for i in range(len(lines)): + match_count = 0 + for i in range(1, len(lines)): if enable_logging: test.log.info("line %s: %s", i, lines[i]) if not ethertype2: if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: - sum += 1 + match_count += 1 if vlan_tag and vlan_tag not in lines[i - 1]: if "too much work for irq" in lines[i - 1]: continue @@ -102,18 +102,18 @@ and ethertype2 in lines[i - 1] ): - sum += 1 + match_count += 1 if vlan_tag not in lines[i - 1] or vlan_tag2 not in lines[i - 1]: if "too much work for irq" in lines[i - 1]: continue @@ -113,7 +113,7 @@ - if sum == 0: + if match_count == 0: test.fail( "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) ) - test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", - iface_name, sum, ethertype, + test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies", + iface_name, match_count,
158-158: Replaceeval()withast.literal_eval()for security.Using
eval()on test parameters is unsafe as it can execute arbitrary code. Useast.literal_eval()to safely parse dictionary literals.Apply this diff:
+import ast + @@ - iface_dict = eval(params.get("iface_dict")) + iface_dict = ast.literal_eval(params.get("iface_dict"))
223-226: Tcpdump blocking depends on cfg fix.The
sendline()calls will block the shell if the tcpdump command in the configuration file isn't backgrounded. This issue is paired with the configuration file problem.After fixing
tcpdump_cmdin the configuration file to include backgrounding (append2>&1 &), consider adding a brief wait and optionally verifying the tcpdump process is running.
345-350: Guard cleanup against uninitialized variables.The finally block accesses
session,host_bridge_iface, andhost_bridgeswithout checking if they were successfully initialized. If the test fails early (e.g., before line 163, 172, or 178), cleanup will raiseNameErrorand mask the original failure. Additionally, line 346 redundantly cleans upguest_pathwhich is already done at line 343.Apply this diff:
+ session = None + host_bridge_iface = None + host_bridges = None + try: vm = env.get_vm(params["main_vm"]) @@ finally: - session.cmd("rm -rf %s" % guest_path) - session.close() + if session: + try: + session.close() + except Exception as e: + test.log.warning(f"Session cleanup failed: {e}") + virsh.destroy(vm_name) - host_bridge_iface.down() - host_bridges.del_bridge(brname) + + if host_bridge_iface: + try: + host_bridge_iface.down() + except Exception as e: + test.log.warning(f"Bridge interface cleanup failed: {e}") + + if host_bridges: + try: + host_bridges.del_bridge(brname) + except Exception as e: + test.log.warning(f"Bridge deletion failed: {e}")Based on learnings
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
230-232: Consider cleaning up VLAN interfaces in finally block.The test creates 802.1ad and 802.1q VLAN interfaces (
advlan_ifnameat line 232 andqvlan_ifnameat line 286) but doesn't explicitly remove them in the finally block. If the test fails partway through, these interfaces may persist on the host.Consider adding VLAN interface cleanup to the finally block:
finally: if session: try: session.close() except Exception as e: test.log.warning(f"Session cleanup failed: {e}") virsh.destroy(vm_name) # Clean up VLAN interfaces if 'advlan_ifname' in locals() and advlan_ifname: try: process.run(f"ip link del {advlan_ifname}", ignore_status=True, shell=True) except Exception as e: test.log.warning(f"Failed to delete {advlan_ifname}: {e}") if 'qvlan_ifname' in locals() and qvlan_ifname: try: process.run(f"ip link del {qvlan_ifname}", ignore_status=True, shell=True) except Exception as e: test.log.warning(f"Failed to delete {qvlan_ifname}: {e}") if host_bridge_iface: try: host_bridge_iface.down() except Exception as e: test.log.warning(f"Bridge interface cleanup failed: {e}") if host_bridges: try: host_bridges.del_bridge(brname) except Exception as e: test.log.warning(f"Bridge deletion failed: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg(1 hunks)libvirt/tests/deps/linux_qinq/qinq.sh(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/deps/linux_qinq/qinq.sh
🧰 Additional context used
🧠 Learnings (3)
📚 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/cfg/virtual_network/qemu/bridge_qinq.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/bridge_qinq.py
📚 Learning: 2025-11-03T13:30:16.458Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6531
File: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py:64-67
Timestamp: 2025-11-03T13:30:16.458Z
Learning: In tp-libvirt tests, prefer using a list of cleanup lambda functions that are appended when actions requiring cleanup are performed, then executed in a finally block with individual error handling. Example: `cleanup_actions.append(lambda: undo_action())` followed by iterating through the list in reverse order in the finally block.
Applied to files:
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
🪛 Ruff (0.14.4)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
135-135: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
155-155: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file"
(S108)
158-158: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
|
@yanglei-rh hi yanglei This pr is updated , and PASSED on kar. I think libvirt-ci can revoke kar and run this test PASS,Please help review it from qemu side , Thanks very much. |
nanli1
changed the title
| tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s" | ||
| tcpdump_log = "/tmp/tcpdump-%s.log" | ||
| get_tcpdump_log_cmd = "cat ${tcpdump_log}" | ||
| advlan_name = "${private_bridge}-vl${vlan_id}" | ||
| add_advlan_cmd = "ip link add link ${private_bridge} name ${advlan_name} type vlan proto 802.1ad id ${vlan_id}" |
There was a problem hiding this comment.
TO AI: setsid could also run a cmd in background.
| vlan_id = 10 | ||
| L1tag_iface = "v1v${vlan_id}" | ||
| L1tag_iface_ip = "192.168.10.10" | ||
| tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s" |
There was a problem hiding this comment.
Suggest update it to setsid tcpdump -xxvvleni %s > %s 2> /dev/null in case the stderr msg is sent to terminal.
|
|
||
| # Ping guest from host via 802.1ad vlan interface | ||
| test.log.info("Start ping test from host to %s via %s", L1tag_iface_ip, advlan_ifname) | ||
| ping_count = int(params.get("ping_count")) |
| L1tag_iface_ip, | ||
| ping_count, | ||
| interface=advlan_ifname, | ||
| timeout=float(ping_count) * 1.5, |
|
|
||
| # Set IP on L2 tag on the guest interface with vid 20 | ||
| L2tag_iface = params["L2tag_iface"] | ||
| L2tag_iface_ip = params["L2tag_iface_ip"] |
There was a problem hiding this comment.
params.get() for line 266, 267
| test.log.info("Creating %dMB file on host", file_size) | ||
| process.run(cmd) | ||
| test.log.info("Transferring file host -> guest, timeout: %ss", transfer_timeout) | ||
| shell_port = int(params.get("shell_port", 22)) |
There was a problem hiding this comment.
shell_port will be used as str type. No need to convert it
| ) | ||
| if not compare_host_guest_md5sum(): | ||
| test.fail("md5sum mismatch on guest and host") | ||
| session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) |
There was a problem hiding this comment.
Move it to line 346 and replace that line
nanli1
force-pushed
the
add_case_for_basic_QinQ_check_test_checking
branch
from
4a80859 to
6470f0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (4)
150-150: Critical:eval()security issue not fixed.Using
eval()on test parameters is unsafe. This was flagged in previous reviews and by static analysis (S307).Apply this diff:
+ import ast + # ... earlier code ... - iface_dict = eval(params.get("iface_dict")) + try: + iface_dict = ast.literal_eval(params.get("iface_dict")) + except (ValueError, SyntaxError) as e: + test.error(f"Invalid iface_dict parameter: {e}")
336-341: Major: Cleanup accesses uninitialized variables.The finally block unconditionally uses
session,host_bridge_iface, andhost_bridges. If the test fails early (e.g., login fails at line 155/181, or bridge creation fails at line 164), cleanup will raiseNameErrororAttributeError, masking the original failure.Initialize variables before
tryand guard cleanup:+ session = None + host_bridge_iface = None + host_bridges = None + try: vm = env.get_vm(params.get("main_vm")) # ... rest of try block ... finally: - session.cmd("rm -rf %s" % guest_path) - session.close() + if session: + try: + session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) + session.close() + except Exception as e: + test.log.warning("Session cleanup failed: %s", e) + virsh.destroy(vm_name) - host_bridge_iface.down() - host_bridges.del_bridge(brname) + + if host_bridge_iface: + try: + host_bridge_iface.down() + except Exception as e: + test.log.warning("Bridge interface cleanup failed: %s", e) + + if host_bridges and brname: + try: + host_bridges.del_bridge(brname) + except Exception as e: + test.log.warning("Bridge deletion failed: %s", e)Note: Based on learnings, VM cleanup is handled by the test framework's teardown, so
virsh.destroy()may be optional.Based on learnings.
336-341: Missing cleanup for VLAN interfaces.The finally block doesn't clean up the 802.1ad (
advlan_ifname) and 802.1q (qvlan_ifname) interfaces created during the test. These should be removed to avoid polluting the host network configuration.Add cleanup for VLAN interfaces:
+ advlan_ifname = None + qvlan_ifname = None + try: + advlan_ifname = params.get("advlan_name") + qvlan_ifname = params.get("qvlan_name", "") # ... rest of try block ... finally: if session: try: session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) session.close() except Exception as e: test.log.warning("Session cleanup failed: %s", e) virsh.destroy(vm_name) + # Clean up VLAN interfaces + if advlan_ifname: + try: + process.run(f"ip link del {advlan_ifname}", ignore_status=True) + except Exception as e: + test.log.warning("Failed to delete advlan interface: %s", e) + + if qvlan_ifname: + try: + process.run(f"ip link del {qvlan_ifname}", ignore_status=True) + except Exception as e: + test.log.warning("Failed to delete qvlan interface: %s", e) + if host_bridge_iface: try: host_bridge_iface.down() except Exception as e: test.log.warning("Bridge interface cleanup failed: %s", e)
75-112: Critical: Off-by-one error and shadowed built-in not fixed.Two critical issues from previous reviews remain unresolved:
Off-by-one: Loop starts at
i=0but accesseslines[i-1]throughout (lines 81, 83, 84, 92, 100, 101, 104, 105). Wheni=0, this wraps tolines[-1](last line), corrupting the match logic.Shadows built-in: Variable
sum(line 75, 82, 103, 112) shadows Python's built-insum()function.Apply this diff:
- sum = 0 - for i in range(len(lines)): + match_count = 0 + for i in range(1, len(lines)): if enable_logging: - test.log.info("line %s: %s", i + 1, lines[i]) - test.log.info("line %s: %s", i, lines[i]) + test.log.info("line %s: %s", i + 1, lines[i]) if not ethertype2: if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: - sum += 1 + match_count += 1 if vlan_tag and vlan_tag not in lines[i - 1]: if "too much work for irq" in lines[i - 1]: continue else: test.fail( "in %s tcpdump log, there should be vlan " "tag %s" % (iface_name, vlan_tag) ) elif not vlan_tag: if "vlan" in lines[i - 1]: test.fail( "in %s tcpdump log, there should not be " "vlan tag" % iface_name ) else: if ( "ICMP echo re" in lines[i] and ethertype in lines[i - 1] and ethertype2 in lines[i - 1] ): - sum += 1 + match_count += 1 if vlan_tag not in lines[i - 1] or vlan_tag2 not in lines[i - 1]: if "too much work for irq" in lines[i - 1]: continue else: test.fail( "in %s tcpdump log, there should be vlan " "tag %s" % (iface_name, vlan_tag) ) - if sum == 0: + if match_count == 0: test.fail( "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) ) - test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", - iface_name, sum, ethertype, + test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", + iface_name, match_count, ethertype, f" and {ethertype2}" if ethertype2 else "", f" with vlan tags {vlan_tag}{vlan_tag2 if vlan_tag2 else ''}" if vlan_tag else "")
🧹 Nitpick comments (5)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (5)
145-145: Useparams.get_numeric()for numeric parameters.Apply this diff:
- file_size = int(params.get("file_size", "4096")) + file_size = params.get_numeric("file_size", 4096)As per coding guidelines (past review from qiankehan).
234-234: Redundantint()conversion.
params.get_numeric()already returns a numeric type; theint()wrapper is unnecessary.Apply this diff:
- ping_count = int(params.get_numeric("ping_count")) + ping_count = params.get_numeric("ping_count")
291-291: Inconsistent timeout calculation.Line 239 uses
float(ping_count) * 1.5, but line 291 omits thefloat()conversion. For consistency and explicitness:Apply this diff:
- timeout=ping_count * 1.5, + timeout=float(ping_count) * 1.5,
11-11: Unused import.The
remodule is imported but never used. This was noted in a previous review.Apply this diff:
import os -import re import timeAs per coding guidelines (past review from qiankehan).
153-153: Reusevm_namevariable.Line 132 already retrieves
params.get('main_vm')intovm_name. Reuse it for consistency.Apply this diff:
- vm = env.get_vm(params.get("main_vm")) + vm = env.get_vm(vm_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg(1 hunks)libvirt/tests/deps/linux_qinq/qinq.sh(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libvirt/tests/deps/linux_qinq/qinq.sh
- libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg
🧰 Additional context used
🧠 Learnings (3)
📚 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/bridge_qinq.py
📚 Learning: 2025-11-03T13:30:16.458Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6531
File: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py:64-67
Timestamp: 2025-11-03T13:30:16.458Z
Learning: In tp-libvirt tests, prefer using a list of cleanup lambda functions that are appended when actions requiring cleanup are performed, then executed in a finally block with individual error handling. Example: `cleanup_actions.append(lambda: undo_action())` followed by iterating through the list in reverse order in the finally block.
Applied to files:
libvirt/tests/src/virtual_network/qemu/bridge_qinq.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/bridge_qinq.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
147-147: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file"
(S108)
150-150: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
🔇 Additional comments (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
244-248: Inconsistent packet loss tolerance.The first ping test (line 244) requires zero packet loss, while the second (line 296) tolerates up to 5% loss. If this is intentional (e.g., due to QinQ overhead or nested VLAN complexity), please add a comment explaining the rationale. Otherwise, use consistent thresholds.
Consider documenting:
package_lost = utils_test.get_loss_ratio(output) + # Allow up to 5% loss for nested QinQ (802.1ad + 802.1q) due to potential overhead if package_lost >= 5: test.fail(Or use consistent zero-loss check if both should be the same.
Also applies to: 296-300
| session.sendline(L1tag_tcpdump_cmd) | ||
| time.sleep(2) | ||
| session.sendline(first_nic_tcpdump_cmd) | ||
| time.sleep(5) |
There was a problem hiding this comment.
tcpdump commands not backgrounded; may block shell.
sendline() without backgrounding (& suffix) leaves the shell busy under tcpdump. While the time.sleep() calls may mask the issue in practice, subsequent commands could be unreliable if tcpdump doesn't fully start.
Ensure the tcpdump commands in the config include & at the end:
# In cfg file, tcpdump_cmd should end with &
tcpdump_cmd = "tcpdump -i %s -vv -n -XX > %s 2>&1 &"Or verify the process started:
session.sendline(L1tag_tcpdump_cmd)
- time.sleep(2)
+ time.sleep(2)
+ # Verify tcpdump is running
+ if "tcpdump" not in session.cmd_output("ps aux | grep tcpdump | grep -v grep"):
+ test.error("tcpdump failed to start on %s" % L1tag_iface)Similar changes apply to lines 269-274.
Also applies to: 269-274
nanli1
force-pushed
the
add_case_for_basic_QinQ_check_test_checking
branch
from
6470f0d to
6849242
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg (1)
20-21: tcpdump still not backgrounded; session may block on tcpdump.
tcpdump_cmdlacks a trailing&, sosession.sendline()will leave the shell occupied by tcpdump and can break subsequent commands, even withsetsid. Keep stderr redirection if desired but ensure the process is backgrounded.Suggested change:
- tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s 2> /dev/null" + tcpdump_cmd = "setsid tcpdump -xxvvleni %s > %s 2> /dev/null &"libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (3)
71-118: Fix tcpdump parsing off‑by‑one and avoid shadowingsum().The loop starts at 0 but uses
lines[i - 1], so fori == 0it reads the last line and may mis-detect matches. Alsosumshadows the built‑in, and the double logging prints the same line twice.Suggested change:
- lines = session.cmd_output( - get_tcpdump_log_cmd, timeout=300, safe=True).strip().splitlines() - sum = 0 - for i in range(len(lines)): - if enable_logging: - test.log.info("line %s: %s", i + 1, lines[i]) - test.log.info("line %s: %s", i, lines[i]) + lines = session.cmd_output( + get_tcpdump_log_cmd, timeout=300, safe=True + ).strip().splitlines() + matches = 0 + for i in range(1, len(lines)): + if enable_logging: + test.log.info("line %s: %s", i + 1, lines[i]) @@ - if not ethertype2: - if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: - sum += 1 + if not ethertype2: + if "ICMP echo re" in lines[i] and ethertype in lines[i - 1]: + matches += 1 @@ - if ( - "ICMP echo re" in lines[i] - and ethertype in lines[i - 1] - and ethertype2 in lines[i - 1] - ): - sum += 1 + if ( + "ICMP echo re" in lines[i] + and ethertype in lines[i - 1] + and ethertype2 in lines[i - 1] + ): + matches += 1 @@ - if sum == 0: + if matches == 0: test.fail( "in %s tcpdump log, ethertype is not %s" % (iface_name, ethertype) ) - test.log.info("tcpdump result check passed for %s - found %d ICMP echo replies with expected ethertype %s%s%s", - iface_name, sum, ethertype, + test.log.info( + "tcpdump result check passed for %s - found %d ICMP echo replies with " + "expected ethertype %s%s%s", + iface_name, + matches, + ethertype, @@ - f" with vlan tags {vlan_tag}{vlan_tag2 if vlan_tag2 else ''}" if vlan_tag else "") + f" with vlan tags {vlan_tag}{vlan_tag2 if vlan_tag2 else ''}" + if vlan_tag + else "", + )
131-150: Avoidevalon params; use structured parsing.
iface_dict = eval(params.get("iface_dict"))is unsafe and triggers Ruff S307. Use the framework’s structured access instead.Suggested change (preferred in this framework):
- iface_dict = eval(params.get("iface_dict")) + iface_dict = params.get_dict("iface_dict")If
params.get_dictis not available in your version, fall back toast.literal_evalfrom the stdlib instead ofeval.
151-339: Harden cleanup infinallyand delete VLAN/bridge resources on teardown.The
session,host_bridge_iface, andhost_bridgesvariables are conditionally assigned inside thetryblock (lines 154, 166, 169, 180). If any setup step fails before these assignments (e.g., login timeout, bridge creation failure), thefinallyblock will raiseAttributeErrororNameErrorwhen attempting to access them (lines 336-340), masking the original failure. Additionally, the 802.1ad VLAN interface (advlan_ifname, created line 222) and 802.1q VLAN interface (qvlan_ifname, created line 278) are never deleted, causing resource leaks into subsequent tests.+ session = None + host_bridge_iface = None + host_bridges = None + advlan_ifname = None + try: vm = env.get_vm(params.get("main_vm")) vm.verify_alive() session = vm.wait_for_login(timeout=login_timeout) @@ host_bridges = utils_net.Bridge() @@ advlan_ifname = params.get("advlan_name") @@ finally: + # Guest cleanup + if session is not None: + try: + session.cmd("rm -rf %s" % guest_path, ignore_all_errors=True) + session.close() + except Exception as detail: + test.log.warning("Session cleanup failed: %s", detail) + + # VM cleanup + if vm_name: + try: + virsh.destroy(vm_name) + except Exception as detail: + test.log.warning("VM destroy failed: %s", detail) + + # VLAN interface cleanup + if advlan_ifname: + process.run("ip link del %s" % advlan_ifname, ignore_status=True) + if qvlan_ifname: + process.run("ip link del %s" % qvlan_ifname, ignore_status=True) + + # Bridge cleanup + if host_bridge_iface is not None: + try: + host_bridge_iface.down() + except Exception as detail: + test.log.warning("Bridge interface down failed: %s", detail) + if host_bridges is not None and brname: + try: + host_bridges.del_bridge(brname) + except Exception as detail: + test.log.warning("Bridge delete failed: %s", detail) - session.cmd("rm -rf %s" % guest_path) - session.close() - virsh.destroy(vm_name) - host_bridge_iface.down() - host_bridges.del_bridge(brname)
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (1)
135-147: Static /var/tmp guest_path is a minor security smell (S108).The default
guest_path/var/tmp/transferred_filelives in a world‑writable directory, which static analysis flags (S108). In tests this is usually acceptable, but if you want to quiet the warning and avoid symlink issues, consider using a per‑test unique path (e.g. under the guest’s/tmpwith a random suffix) coordinated with the in‑guest commands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libvirt/tests/cfg/virtual_network/qemu/bridge_qinq.cfg(1 hunks)libvirt/tests/deps/linux_qinq/qinq.sh(1 hunks)libvirt/tests/src/virtual_network/qemu/bridge_qinq.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/deps/linux_qinq/qinq.sh
🧰 Additional context used
🧠 Learnings (4)
📚 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/cfg/virtual_network/qemu/bridge_qinq.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/bridge_qinq.py
📚 Learning: 2025-11-03T13:30:16.458Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6531
File: libvirt/tests/src/virtual_network/qemu/pktgen_burst_mode_test.py:64-67
Timestamp: 2025-11-03T13:30:16.458Z
Learning: In tp-libvirt tests, prefer using a list of cleanup lambda functions that are appended when actions requiring cleanup are performed, then executed in a finally block with individual error handling. Example: `cleanup_actions.append(lambda: undo_action())` followed by iterating through the list in reverse order in the finally block.
Applied to files:
libvirt/tests/src/virtual_network/qemu/bridge_qinq.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/bridge_qinq.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py (2)
libvirt/tests/src/serial/serial_functional.py (1)
wait_for_login(235-258)libvirt/tests/src/virtual_network/iface_bridge.py (1)
ping(83-98)
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/bridge_qinq.py
146-146: Probable insecure usage of temporary file or directory: "/var/tmp/transferred_file"
(S108)
149-149: Use of possibly insecure function; consider using ast.literal_eval
(S307)
xxxx-95976: [virtual network][virual-nic-device] Basic QinQ test - 10x4094 check -- only for linux Signed-off-by: nanli <[email protected]>
nanli1
force-pushed
the
add_case_for_basic_QinQ_check_test_checking
branch
from
6849242 to
6cb6087
Compare
|
@qiankehan hi hhan, All your comment has been updated, many good catch, Thanks a lot , Please help re-review |
2 participants
xxxx-95976: [virtual network][virual-nic-device] Basic QinQ test - 10x4094 check -- only for linux
Signed-off-by: nanli [email protected]
hi all reviewers, There is one migrate_from_qemu label set on this patch
This patch is migrated from qemu side, so except the libvirt define xml part , other parts are just copying from qemu side, So Scrum team decide we don't need to review it too detail and focus on libvirt part, Thanks all
Summary by CodeRabbit
Tests
Chores