-
Notifications
You must be signed in to change notification settings - Fork 182
vsock_transfer_data:Add script to test transfer data via vsock #4422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new Windows-only VSock data transfer test consisting of a config file Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/vsock_transfer_data.cfg(1 hunks)qemu/tests/vsock_transfer_data.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/vsock_transfer_data.py (1)
provider/win_driver_utils.py (1)
get_driver_inf_path(123-155)
🔇 Additional comments (1)
qemu/tests/cfg/vsock_transfer_data.cfg (1)
1-21: Configuration is correctly implemented.The vsock_transfer_data test configuration properly separates host and guest commands:
ddandsocatexecute on the Linux host viaprocess.system_output(), whilecertutiland the viosocklib test tool execute in the Windows guest viasession.cmd_output(). The test module correctly retrieves and applies these parameters, with thesend_data_cmdproperly formatted with the guest CID at runtime (line 79 of vsock_transfer_data.py). All configuration parameters are used as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (6)
qemu/tests/vsock_transfer_data.py (6)
18-21: Critical race condition: server_ready signal set prematurely.This issue was previously flagged and remains unresolved. The
server_readyevent is set at line 19 before the server command at line 20 actually starts executing or becomes ready to accept connections. This creates a race where the client thread may attempt to connect before the server is listening.Additionally:
session.cmd_output(cmd)has no timeout and will block indefinitely if the command hangs- Exceptions are not caught, so thread failures will be silent
</review_comment_end -->
23-27: Add timeout and error handling; reconsider hard-coded sleep.This issue was previously flagged and remains unresolved. The client thread has several issues:
- Line 25: The hard-coded 1-second sleep is a brittle timing assumption that may be insufficient if the server takes longer to initialize
- Line 26:
process.system_output(cmd)has no timeout and will hang indefinitely if the command stalls- No exception handling means errors are silently lost
</review_comment_end -->
46-47: Update docstring to match actual test purpose.This issue was previously flagged. The docstring says "Vsock bridge service test" but the test is actually for vsock data transfer validation, as indicated by the test name and implementation.
</review_comment_end -->
64-68: Critical: Validate array access and device retrieval.These issues were previously flagged and remain unresolved:
Line 67 uses
split()[0]without verifying the list is non-empty, which will raise anIndexErrorifparams["vsocks"]is empty or whitespace-only.Line 68 retrieves a device and parameter without validating they exist, which can result in
Noneor raise exceptions if the vsock device is not properly configured.</review_comment_end -->
70-74: Add validation and ensure cleanup of generated files.These issues were previously flagged and remain unresolved:
Line 72 performs string manipulation (
path.rfind("\\")) without validating that the backslash exists, potentially resulting in incorrect paths.Line 73 copies the test tool without verifying the source file exists.
Line 74 generates a 100MB test file on the host without any cleanup mechanism, which will accumulate over repeated test runs.
</review_comment_end -->
82-90: Critical: Validate array access and normalize MD5 comparison.These issues were previously flagged and remain unresolved:
Lines 84 and 86 perform array access without validation:
- Line 84:
split()[0]will raiseIndexErrorif md5sum output is empty or malformed- Line 86:
splitlines()[1]will raiseIndexErrorif certutil output has fewer than 2 linesAdditionally, line 87 compares MD5 hashes as raw strings without normalization (case-insensitive, strip whitespace), which could cause false failures.
</review_comment_end -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/vsock_transfer_data.cfg(1 hunks)qemu/tests/vsock_transfer_data.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/vsock_transfer_data.py (1)
provider/win_driver_utils.py (1)
get_driver_inf_path(123-155)
8b9ff9c to
2215553
Compare
ID:4798 Signed-off-by: Leidong Wang <[email protected]>
|
JOB LOG : /root/avocado/job-results/job-2025-12-12T00.31-37ee4b8/job.log |
|
Hi @JinLiul Could you please help review this PR?Thanks. |
|
LGTM. |
|
@leidwang Do we have vsock data transfer test existed for Linux? Is that possible to reuse some of the test? |
Yes, there are. However, Windows testing and Linux testing have many different configurations and steps, and currently Linux test scripts combine many different vsock test scenarios. Adding Windows vsock testing to the current Linux test script would make the Linux script very complex, so I decided to write a separate script for Windows testing.Thanks! |
ID:4798
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.