Add case to support download/upload file over ipv6 #6673
Conversation
Automate VIRT-95920 download/upload files via ipv6 Signed-off-by: Lei Yang <[email protected]>
WalkthroughThis pull request introduces a new IPv6 file transfer test suite for libvirt/QEMU environments. It adds a configuration file that defines VM lifecycle management, test parameters (image size, timeouts), IPv6 addressing variants (including link-local address scenarios), and OS-specific temporary directories. The accompanying Python test script implements the test execution logic, orchestrating VM boot sequences, IPv6 address discovery, multi-directional file transfers (host-to-guest, guest-to-guest, guest-to-host), and end-to-end data integrity validation through MD5 comparisons. The implementation supports both Linux and Windows guests with timeout configurability and includes comprehensive error handling and cleanup. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20-25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 5
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py (1)
182-182: Use process.system instead of system_output for cleanup.Line 182 uses
process.system_output()for anrmcommand but doesn't capture or use the output. Useprocess.system()instead, which is the appropriate function for commands where output is not needed.Apply this diff:
- process.system_output("rm -rf %s" % host_path, timeout=timeout) + process.system("rm -rf %s" % host_path, timeout=timeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/transfer_file_over_ipv6.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py(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/transfer_file_over_ipv6.py
🪛 Ruff (0.14.5)
libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py
87-87: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (6)
libvirt/tests/cfg/virtual_network/qemu/transfer_file_over_ipv6.cfg (1)
18-24: LGTM! Well-structured IPv6 test variants.The configuration properly defines two test scenarios for IPv6 file transfers: remote addresses and link-local addresses. The host-specific guest interface flag for older RHEL versions is a good accommodation for platform differences.
libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py (5)
1-22: LGTM! Clean imports and well-documented function.The imports are appropriate for the test requirements, and the docstring clearly describes the test flow and parameters.
23-33: LGTM! Parameter extraction is correct.The parameters are properly extracted with appropriate type conversions and sensible defaults where applicable.
86-87: Acknowledge security note: shell=True is acceptable here.The static analysis tool flags
shell=Trueas a security risk. However, in this test framework context, it's acceptable because:
- The
dd_cmdcomes from a trusted configuration file- The parameters (
host_path,filesize) are either generated securely or type-checked- This is test infrastructure code, not production code handling untrusted input
184-197: LGTM! Proper cleanup with error handling.The finally block properly cleans up test artifacts on both host and guests, with appropriate error suppression to ensure cleanup continues even if individual operations fail. Session cleanup is also handled correctly.
67-73: Fix syntax error: missing closing parenthesis.Line 73 is missing a closing parenthesis, which will cause a syntax error.
Apply this diff:
addresses[vm] = utils_net.get_guest_ip_addr( sessions[vm], vm.get_mac_address(), params.get("os_type"), ip_version="ipv6", linklocal=link_local_ipv6_addr, - ) + )Likely an incorrect or invalid review comment.
| Linux: | ||
| tmp_dir = "/var/tmp/" | ||
| Windows: | ||
| tmp_dir = "C:\\" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Windows path escaping.
The Windows temporary directory path uses a single backslash. Depending on how the configuration parser handles this string, it may need to be escaped as C:\\\\ or use a raw string format.
Run the following script to check how similar Windows paths are defined in other test configurations:
🏁 Script executed:
#!/bin/bash
# Description: Check Windows tmp_dir path format in other configuration files
# Search for Windows tmp_dir definitions in other test configs
rg -n --type=cfg -A1 -B1 'Windows:' libvirt/tests/cfg/ | rg 'tmp_dir'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Description: Find Windows path definitions in configuration files
# First, find all .cfg files in the test directory
echo "=== Finding .cfg files with Windows references ==="
fd -e cfg -x rg -l 'Windows' {} \;
# Search for tmp_dir patterns in all .cfg files
echo -e "\n=== Searching for tmp_dir patterns in .cfg files ==="
fd -e cfg -x rg -n 'tmp_dir' {} +
# Examine the specific file to understand context
echo -e "\n=== Content of transfer_file_over_ipv6.cfg ==="
cat libvirt/tests/cfg/virtual_network/qemu/transfer_file_over_ipv6.cfgLength of output: 47424
🏁 Script executed:
#!/bin/bash
# Description: Find the configuration file parser
# Search for files that parse or load .cfg files
echo "=== Finding .cfg file parser implementation ==="
rg -n -i 'class.*config|def.*parse.*cfg|\.cfg' --type=py -A3 -B3 | head -100
# Look for configuration loading functions
echo -e "\n=== Searching for config parsing in Python files ==="
rg -n 'load.*config|parse.*cfg|read.*\.cfg' --type=py -C2 | head -80
# Find import statements related to config/yaml
echo -e "\n=== Searching for YAML or config parsers ==="
rg -n 'import.*yaml|import.*config|from.*yaml|from.*config' --type=py | head -30Length of output: 18756
🏁 Script executed:
#!/bin/bash
# Description: Find where .cfg files are loaded
# Search for where .cfg configuration files are loaded
echo "=== Finding where .cfg files are loaded ==="
rg -n 'load.*\.cfg|parse.*\.cfg|open.*\.cfg' --type=py -B2 -A2 | head -60
# Look for YAML loading specifically
echo -e "\n=== Finding yaml.load usage ==="
rg -n 'yaml\.load|yaml\.safe_load' --type=py -B3 -A3
# Search for how params are retrieved from config (params.get pattern)
echo -e "\n=== Looking for params dictionary source ==="
rg -n 'params\s*=' --type=py | grep -E 'config|cfg|load' | head -20Length of output: 2627
🏁 Script executed:
#!/bin/bash
# Description: Find where .cfg files are loaded and how params are parsed
# Look for file loading patterns
echo "=== Searching for test configuration loading ==="
find . -name "*.py" -type f | xargs grep -l "\.cfg" | head -10 | xargs grep -n "open\|load" | grep -i "cfg\|config" | head -40
# Search for the actual config file parser
echo -e "\n=== Looking for configuration module ==="
rg -n 'class.*Config|def.*load_config' --type=py -A5 | head -60
# Check if there's documentation about escape sequences
echo -e "\n=== Check for escape sequence handling in tests ==="
rg -n 'escape|backslash|\\\\' --type=py libvirt/tests/src/virtual_network/ | head -20Length of output: 143
🌐 Web query:
YAML string escaping rules backslash double quotes
💡 Result:
Short summary — rules for backslash and double quotes in YAML strings:
-
Single-quoted scalars: backslash has no special meaning (it is a literal character). The only escape is to double a single quote: 'He''s'. Use single quotes when you want literal backslashes. (yaml.info)
-
Double-quoted scalars: backslash introduces escape sequences (JSON/C-like). You must escape the double-quote (") and the backslash (\). Supported escapes include common ones (\n, \t, \r, \b, \f, \a, \0), hex/unicode (\xNN, \uNNNN, \UNNNNNNNN), and special YAML escapes; an escaped line break is ignored (allows multiline). Invalid escape sequences are an error. Examples:
- "She said "Hi""
- "C:\Program Files\App"
- "Line1\nLine2" (yaml.org)
References:
- YAML 1.2 / 1.2.2 spec — escaped characters & double-quoted scalars. (yaml.org)
- Practical summary (yaml.info) — quoting behavior and examples. (yaml.info)
If you want, I can list the full escape sequence table from the spec or give parser-specific notes (some YAML parsers differ slightly).
Citations:
- 1: https://www.yaml.info/learn/quote?utm_source=openai
- 2: https://yaml.org/spec/1.2.2/?utm_source=openai
- 3: https://yaml.org/spec/1.2.2/?utm_source=openai
- 4: https://www.yaml.info/learn/quote?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Description: Find how tmp_dir is used in tests
echo "=== Finding where tmp_dir is used in source code ==="
rg -n 'tmp_dir' --type=py -B2 -A2 | head -80
echo -e "\n=== Searching for file transfer test code ==="
rg -n 'transfer_file_over_ipv6\|tmp_dir.*Windows' --type=py -B3 -A3 | head -50
echo -e "\n=== Checking for Windows path handling in tests ==="
rg -n 'C:\\|Windows.*path|tmp_dir' --type=py libvirt/tests/src/virtual_network/ -B2 -A2 | head -40Length of output: 9908
Fix Windows tmp_dir path escaping from "C:\\" to "C:\\\\"
In the configuration file, the Windows tmp_dir path uses insufficient escaping for YAML double-quoted strings. In YAML double-quoted strings, backslash introduces escape sequences, and the backslash itself must be escaped as \\.
The current value "C:\\" is parsed as C:\ (single backslash). When concatenated with filenames in the test code (line 81-82 of transfer_file_over_ipv6.py), this produces invalid paths like C:\src-xxx.
To produce the correct path separator, use "C:\\\\" which parses to C:\\ (double backslash), resulting in valid concatenated paths like C:\\src-xxx.
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/transfer_file_over_ipv6.cfg around
line 17, the tmp_dir value uses the YAML double-quoted string "C:\\" which is
parsed as C:\ (single backslash) and causes invalid concatenated paths; change
the value to "C:\\\\" so YAML parses it as C:\\ (double backslash) and
concatenations like C:\\src-xxx become valid.
| def get_file_md5sum(file_name, session, timeout): | ||
| """ | ||
| Get file md5sum from guest. | ||
| """ | ||
| test.log.info("Get md5sum of the file:'%s'", file_name) | ||
| s, o = session.cmd_status_output("md5sum %s" % file_name, timeout=timeout) | ||
| if s != 0: | ||
| test.error("Get file md5sum failed as %s" % o) | ||
| return re.findall(r"\w{32}", o)[0] |
There was a problem hiding this comment.
Verify Windows compatibility for MD5 calculation.
The get_file_md5sum function executes md5sum, which is a Linux command. Windows guests may not have this command available by default. The test transfers files to Windows guests (line 93-116) and would fail when calculating MD5 on Windows.
Consider adding OS-specific MD5 command logic:
def get_file_md5sum(file_name, session, timeout):
"""
Get file md5sum from guest.
"""
test.log.info("Get md5sum of the file:'%s'", file_name)
- s, o = session.cmd_status_output("md5sum %s" % file_name, timeout=timeout)
+ if params.get("os_type") == "linux":
+ cmd = "md5sum %s" % file_name
+ else:
+ cmd = "certutil -hashfile %s MD5" % file_name
+ s, o = session.cmd_status_output(cmd, timeout=timeout)
if s != 0:
test.error("Get file md5sum failed as %s" % o)
return re.findall(r"\w{32}", o)[0]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py around
lines 35 to 43, the helper always runs the Linux-only "md5sum" which will fail
on Windows guests; update get_file_md5sum to detect the guest OS (or accept an
is_windows/session.os flag), run a Windows-safe command such as "certutil
-hashfile <file> MD5" or PowerShell "Get-FileHash -Algorithm MD5 <file>" when
the guest is Windows, parse the command output to extract the hex MD5, keep the
existing md5sum branch for Unix, and handle non-zero exit codes with test.error
including the command output.
| if link_local_ipv6_addr is False and addresses[vm] is None: | ||
| test.cancel("Your guest can not get remote IPv6 address.") | ||
| error_context.context( | ||
| "Get ipv6 address of %s: %s" % (vm.name, addresses[vm]), test.log.info | ||
| ) |
There was a problem hiding this comment.
Remove unreachable code after test.cancel.
Lines 76-78 are unreachable because test.cancel() on line 75 raises an exception and exits the test. The error_context.context() call will never execute.
Apply this diff to remove unreachable code and move the context logging before the cancel:
- if link_local_ipv6_addr is False and addresses[vm] is None:
- test.cancel("Your guest can not get remote IPv6 address.")
- error_context.context(
- "Get ipv6 address of %s: %s" % (vm.name, addresses[vm]), test.log.info
- )
+ error_context.context(
+ "Get ipv6 address of %s: %s" % (vm.name, addresses[vm]), test.log.info
+ )
+ if link_local_ipv6_addr is False and addresses[vm] is None:
+ test.cancel("Your guest can not get remote IPv6 address.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if link_local_ipv6_addr is False and addresses[vm] is None: | |
| test.cancel("Your guest can not get remote IPv6 address.") | |
| error_context.context( | |
| "Get ipv6 address of %s: %s" % (vm.name, addresses[vm]), test.log.info | |
| ) | |
| error_context.context( | |
| "Get ipv6 address of %s: %s" % (vm.name, addresses[vm]), test.log.info | |
| ) | |
| if link_local_ipv6_addr is False and addresses[vm] is None: | |
| test.cancel("Your guest can not get remote IPv6 address.") |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py around
lines 74 to 78, the error_context.context() call after test.cancel() is
unreachable because test.cancel() raises and exits; move the
error_context.context(...) call so it runs before calling test.cancel(), then
remove the now-unreachable lines after test.cancel() so logging occurs and no
dead code remains.
| if params.get_boolean("using_guest_interface"): | ||
| dst_interface = inet_name[vm_src] | ||
| else: | ||
| dst_interface = host_ifname |
There was a problem hiding this comment.
Fix interface selection logic for guest-to-guest transfer.
When using_guest_interface is true, the destination interface should be from the destination VM (vm_dst), not the source VM (vm_src). The current logic appears reversed.
Apply this diff:
if params.get_boolean("using_guest_interface"):
- dst_interface = inet_name[vm_src]
+ dst_interface = inet_name[vm_dst]
else:
dst_interface = host_ifname📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if params.get_boolean("using_guest_interface"): | |
| dst_interface = inet_name[vm_src] | |
| else: | |
| dst_interface = host_ifname | |
| if params.get_boolean("using_guest_interface"): | |
| dst_interface = inet_name[vm_dst] | |
| else: | |
| dst_interface = host_ifname |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py around
lines 126 to 129, the branch that sets dst_interface when
params.get_boolean("using_guest_interface") is selecting inet_name[vm_src]
(source VM) incorrectly; change it to use inet_name[vm_dst] so the destination
interface comes from the destination VM. Update that assignment only, ensuring
vm_dst is in scope and used for the guest-to-guest transfer path.
| error_context.context( | ||
| "md5 value of data in %s: %s" % (vm.name, dst_md5), | ||
| test.log.info, | ||
| ) |
There was a problem hiding this comment.
Fix incorrect variable reference in error context.
Line 148 uses vm.name but should use vm_dst.name since we're logging the MD5 value for the destination VM in the guest-to-guest transfer loop.
Apply this diff:
dst_md5 = get_file_md5sum(
dest_path, sessions[vm_dst], timeout=file_md5_check_timeout
)
error_context.context(
- "md5 value of data in %s: %s" % (vm.name, dst_md5),
+ "md5 value of data in %s: %s" % (vm_dst.name, dst_md5),
test.log.info,
)📝 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.
| error_context.context( | |
| "md5 value of data in %s: %s" % (vm.name, dst_md5), | |
| test.log.info, | |
| ) | |
| error_context.context( | |
| "md5 value of data in %s: %s" % (vm_dst.name, dst_md5), | |
| test.log.info, | |
| ) |
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/qemu/transfer_file_over_ipv6.py around
lines 147 to 150, the error_context.context call logs the MD5 for the
destination VM but incorrectly references vm.name; change that reference to
vm_dst.name so the message reads the destination VM's name (i.e., replace
vm.name with vm_dst.name in the formatted string).
2 participants
Automate VIRT-95920 download/upload files via ipv6
ID:LIBVIRTAT-22159
Signed-off-by: Lei Yang [email protected]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.