vsock_driver_check: Add script to test vsock driver #4418
Conversation
WalkthroughAdds a Windows-only VSock driver validation to the QEMU test suite. Introduces Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
qemu/tests/cfg/vsock_driver_check.cfg (1)
19-20: Consider parameterizing the destination directory.The test tool destination is hardcoded to
C:\in both the configuration and test code (line 50 of vsock_driver_check.py). Consider making this configurable for flexibility.qemu/tests/vsock_driver_check.py (4)
30-32: Consider verifying the vsock device presence.The test assumes the VM is configured with a vhost-vsock-pci device (per step 2 in the docstring), but there's no verification. Consider checking the device is present before proceeding with driver installation to provide clearer error messages if misconfigured.
You could verify the device using:
# Verify vsock device is present in VM vsock_id = params.get("vsocks", "").split()[0] if params.get("vsocks") else None if not vsock_id: test.error("No vsock device configured for VM")
34-42: Consider parameterizing the driver name.The driver name is hardcoded as
"viosock". While this is correct for the current use case, parameterizing it (e.g.,params.get("vsock_driver_name", "viosock")) would make the test more flexible.
51-53: Improve error message informativeness.The failure message doesn't show the actual output, making debugging difficult. Consider including the command output in the error message.
output = session.cmd_output(params["vsock_provider_check"]) if params["vsock_provider"] not in output: - test.fail("Not find vsock provider in guest.") + test.fail(f"VSock provider '{params['vsock_provider']}' not found in output: {output}")
32-55: Consider using try-finally for session cleanup.The session is created at line 32 but only closed at line 55. If any intermediate step fails (test.fail() or exception), the session may not be properly closed. Consider wrapping the test logic in a try-finally block to ensure cleanup.
session = vm.wait_for_login() +try: + LOG_JOB.info("Install vsock driver in guest") + install_driver_by_virtio_media( + session, + test, + devcon_path=params["devcon_path"], + media_type=params["virtio_win_media_type"], + driver_name="viosock", + device_hwid=params["viosock_hwid"], + ) -LOG_JOB.info("Install vsock driver in guest") -install_driver_by_virtio_media( - session, - test, - devcon_path=params["devcon_path"], - media_type=params["virtio_win_media_type"], - driver_name="viosock", - device_hwid=params["viosock_hwid"], -) - -LOG_JOB.info("Check vsock driver status in guest") -if session.cmd_status(params["vio_driver_chk_cmd"]): - test.fail("Vsock driver status is not ready.") - -LOG_JOB.info("Check vsock prodiver in guest") -src_file = os.path.join(data_dir.get_deps_dir("virtio_sock"), params["test_tool"]) -vm.copy_files_to(src_file, "C:") -output = session.cmd_output(params["vsock_provider_check"]) -if params["vsock_provider"] not in output: - test.fail("Not find vsock provider in guest.") - -session.close() + LOG_JOB.info("Check vsock driver status in guest") + if session.cmd_status(params["vio_driver_chk_cmd"]): + test.fail("Vsock driver status is not ready.") + + LOG_JOB.info("Check vsock provider in guest") + src_file = os.path.join(data_dir.get_deps_dir("virtio_sock"), params["test_tool"]) + vm.copy_files_to(src_file, "C:") + output = session.cmd_output(params["vsock_provider_check"]) + if params["vsock_provider"] not in output: + test.fail(f"VSock provider '{params['vsock_provider']}' not found in output: {output}") +finally: + session.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/vsock_driver_check.cfg(1 hunks)qemu/tests/vsock_driver_check.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/vsock_driver_check.py (2)
provider/win_driver_utils.py (1)
install_driver_by_virtio_media(159-194)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (7)
qemu/tests/cfg/vsock_driver_check.cfg (3)
13-16: LGTM!The architecture-specific devcon directory mappings are correct for Windows conventions (x86 for 32-bit, amd64 for 64-bit).
6-7: [Your rewritten review comment text here]
[Exactly ONE classification tag]
12-12: Verify the hardware ID quoting pattern.The
viosock_hwiduses mixed quoting:'"PCI\VEN_1AF4&DEV_1053"'. Standard DevCon commands quote hardware IDs with & characters using double quotes (e.g.,"PCI\VEN_1AF4&DEV_1053"), not the single-quoted-outer-double-quoted-inner pattern shown here. Confirm whether this quoting is required by the autotest framework when passing the HWID to driver installation commands, or if it should be simplified to standard double quoting.qemu/tests/vsock_driver_check.py (4)
1-10: LGTM!Imports are appropriate and align with the avocado testing framework and virttest utilities.
12-26: LGTM!The function signature and documentation follow virttest conventions. The test steps are clearly outlined.
44-46: LGTM!The driver status check logic is correct, and the error message is clear.
49-49: Verify the dependency directory exists.The test tool path assumes
virtio_sockexists in the deps directory. Ensure this directory and the test tool file are properly documented as test dependencies.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
qemu/tests/cfg/vsock_driver_check.cfg (1)
1-20: Config block is consistent with other virtio/Windows tests; a couple of details worth confirmingOverall this looks well-aligned with the new
vsock_driver_checktest: Windows-only gating, virtio-win version ranges,devcon_pathconstruction (arch-specificdevcon_dirname), and thedriverquery-basedvio_driver_chk_cmdall fit the usual tp-qemu patterns. I don’t see any blockers here.Two minor points to double‑check:
Line 9:
vsocks = vhost_vsock0– ensure this value matches what the Python test actually expects on your supported hosts (e.g., whether it treats this as a device/node name versus a module/identifier). If the code uses it directly as a device path or name, a mismatch with the real host vsock device could cause the test to fail or silently skip functionality.Line 20:
vsock_provider_check = "C:\${test_tool} /e"assumes the provider test tool is copied toC:\specifically. Ifvsock_driver_check.pyever starts placing the tool elsewhere (or you run with a non‑C system drive), this will drift. You might consider either:
- Adding a separate
test_tool_dirparameter and composing the command in Python, or- Keeping just the executable name here and letting the Python test prepend the directory when building the command.
These are non‑blocking and can be deferred, but tightening them up will make the test a bit more robust to environment changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deps/virtio_sock/viosocklib-test.exeis excluded by!**/*.exe
📒 Files selected for processing (2)
qemu/tests/cfg/vsock_driver_check.cfg(1 hunks)qemu/tests/vsock_driver_check.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/vsock_driver_check.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
qemu/tests/vsock_driver_check.py (1)
28-29: Add error handling for module loading.The module load operation should verify success before proceeding to guest operations.
This issue was already flagged in a previous review.
🧹 Nitpick comments (4)
qemu/tests/vsock_driver_check.py (4)
34-42: Consider parameterizing the driver name.The driver name
"viosock"is hardcoded on line 40. For consistency with other test parameters, consider adding a config parameter likevsock_driver_name = "viosock"and usingparams["vsock_driver_name"].Apply this diff to parameterize the driver name:
install_driver_by_virtio_media( session, test, devcon_path=params["devcon_path"], media_type=params["virtio_win_media_type"], - driver_name="viosock", + driver_name=params.get("vsock_driver_name", "viosock"), device_hwid=params["viosock_hwid"], )And add to the config file (qemu/tests/cfg/vsock_driver_check.cfg):
vsock_driver_name = "viosock"
44-46: Add timeout and improve error message.The driver status check should specify an explicit timeout to prevent potential hangs, and the error message should include the command that was executed for better debugging.
Apply this diff:
LOG_JOB.info("Check vsock driver status in guest") - if session.cmd_status(params["vio_driver_chk_cmd"]): - test.fail("Vsock driver status is not ready.") + chk_cmd = params["vio_driver_chk_cmd"] + if session.cmd_status(chk_cmd, timeout=60): + test.fail(f"Vsock driver not found. Command '{chk_cmd}' failed.")
51-54: Add timeout and improve error message.The provider check should specify an explicit timeout, and the error message should include the actual command output for debugging purposes.
Apply this diff:
- output = session.cmd_output(params["vsock_provider_check"]) + provider_check_cmd = params["vsock_provider_check"] + output = session.cmd_output(provider_check_cmd, timeout=120) if params["vsock_provider"] not in output: - test.fail("Not find vsock provider in guest.") + test.fail( + f"Vsock provider '{params['vsock_provider']}' not found.\n" + f"Command: {provider_check_cmd}\nOutput: {output}" + )
55-55: Consider module cleanup.The session is properly closed, but the
vhost_vsockmodule loaded on line 29 is never explicitly unloaded. Consider whether module cleanup should be added to a finally block or teardown method to ensure proper resource cleanup, or verify that the test framework handles this automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deps/virtio_sock/viosocklib-test.exeis excluded by!**/*.exe
📒 Files selected for processing (2)
qemu/tests/cfg/vsock_driver_check.cfg(1 hunks)qemu/tests/vsock_driver_check.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/vsock_driver_check.py (1)
provider/win_driver_utils.py (1)
install_driver_by_virtio_media(159-194)
🔇 Additional comments (7)
qemu/tests/cfg/vsock_driver_check.cfg (4)
1-7: LGTM! Configuration setup is correct.The test type, Windows constraint, VirtIO media configuration, and version requirements are properly defined.
8-12: LGTM! VSock device configuration is correct.The module name, device identifiers, and hardware IDs for VirtIO vsock (both modern 1053 and transitional 1012 device IDs) are properly configured.
13-17: LGTM! Architecture-specific devcon paths are correctly configured.The configuration properly handles both 32-bit and 64-bit Windows architectures with appropriate devcon.exe paths.
18-20: LGTM! Command definitions are correct.The driver check command using
driverqueryand the provider check tool invocation are properly configured for Windows.qemu/tests/vsock_driver_check.py (3)
1-10: LGTM! Imports and logger setup are correct.All necessary dependencies are imported and the logger is properly configured.
12-26: LGTM! Test function signature and documentation are correct.The function follows the standard QEMU test pattern with proper decorator and clear docstring describing all test steps.
30-32: LGTM! VM initialization follows standard pattern.The VM retrieval, liveness verification, and session establishment follow the correct QEMU test framework pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
qemu/tests/cfg/vsock_driver_check.cfg (1)
10-13: Optional: consider centralising viosock metadata with other VirtIO driversYou’re introducing
driver_name,vsock_name, and aviosock_hwidlist here, while other drivers keep similar metadata inwin_driver_utils.driver_info_dict. For future maintainability, it might be worth adding aviosockentry there and reusing it instead of hard-coding HWIDs and display names in this cfg block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/vsock_driver_check.cfg(1 hunks)qemu/tests/vsock_driver_check.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/vsock_driver_check.py
⏰ 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). (1)
- GitHub Check: Python 3.12
🔇 Additional comments (1)
qemu/tests/cfg/vsock_driver_check.cfg (1)
1-20: Config wiring and parameters look consistent with the Python testThe block defines exactly the parameters
vsock_driver_check.pyexpects (virtio_win_media_type,driver_name,test_tool,devcon_path,viosock_hwid,vio_driver_chk_cmd,vsock_provider), and the HWID format matchesinstall_driver_by_virtio_media’s expectations. From a wiring standpoint this cfg is coherent with the new test.
|
JOB LOG : /root/avocado/job-results/job-2025-12-09T10.04-524cb9a/job.log |
|
Hi @JinLiul Could you please help review this PR?Thanks. |
|
LGTM. |
| linux_modules.load_module("vhost_vsock") | ||
| vm = env.get_vm(params["main_vm"]) | ||
| vm.verify_alive() | ||
| session = vm.wait_for_login() |
There was a problem hiding this comment.
Though it's also fine, but vm.wait_for_login() would covered vm.verify_alive().
There was a problem hiding this comment.
Yes, vm.verify_alive() has been removed.Thanks!
Test vsock driver in Windows VM, will check vsock driver status and provider. ID:4795 Signed-off-by: Leidong Wang <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
qemu/tests/vsock_driver_check.py (2)
54-54: Use os.path for safer path handling.The string manipulation
path[: path.rfind("\\")]has an edge case: ifrfindreturns -1 (backslash not found),path[:-1]incorrectly slices off the last character. Whileget_driver_inf_pathshould always return Windows paths with backslashes, defensive coding is safer.♻️ Refactor using os.path for robustness
Add
osimport at the top:+import os import loggingThen replace the path construction:
- test_tool_path = path[: path.rfind("\\")] + "\\" + test_tool + test_tool_path = os.path.join(os.path.dirname(path), test_tool)
55-57: Add error handling for test tool execution.If the test tool doesn't exist at the constructed path or execution fails,
cmd_outputwill raise an exception, causing the test to crash with an unclear error. Add validation or error handling to provide a clearer failure message.🛡️ Add error handling
LOG_JOB.info("Check vsock provider in guest") path = get_driver_inf_path(session, test, virtio_win_media_type, driver_name) test_tool_path = path[: path.rfind("\\")] + "\\" + test_tool - output = session.cmd_output("%s /e" % test_tool_path) + status, output = session.cmd_status_output("%s /e" % test_tool_path) + if status: + test.fail(f"Failed to execute test tool at {test_tool_path}: {output}") if params["vsock_provider"] not in output: test.fail("Not find vsock provider in guest.")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/vsock_driver_check.cfgqemu/tests/vsock_driver_check.py
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/vsock_driver_check.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/vsock_driver_check.py (1)
provider/win_driver_utils.py (2)
get_driver_inf_path(123-155)install_driver_by_virtio_media(159-194)
3 participants
Test vsock driver in Windows VM, will check vsock driver status and provider.
ID:4795
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.