virtio_win_socket_test: vsock hotplug testing #4429
Conversation
WalkthroughAdds Windows VirtIO Socket test support: introduces a VSockTransfer class and a viosock_test function in provider/win_driver_installer_test.py to perform threaded host–guest data transfers and MD5 verification on Windows guests. Adds a Windows-only test configuration block in qemu/tests/cfg/virtio_win_socket_test.cfg with tool paths, commands, and a hotplug variant. Updates qemu/tests/vsock_hotplug.py to branch on guest OS, invoking viosock_test for Windows guests while retaining existing tool discovery and transfer logic for non-Windows guests; also imports the new Windows test helper. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)qemu/tests/vsock_hotplug.py (3)
provider/win_driver_installer_test.py (1)
🔇 Additional comments (3)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qemu/tests/vsock_hotplug.py (1)
54-77: Fix variable scope issue causing pipeline failure.The variable
device_stris only assigned in theelseblock (line 59) for Linux, but is used unconditionally in thedmesg_checkblock (lines 62-77). Whenos_type == "windows",device_stris never defined, causing aNameErrorand the reported pylint error E0606.🔎 Proposed fix
output = session.cmd_output(check_vsock_cmd) if os_type == "windows": print("%s %s" % (device_pattern, output)) if device_pattern not in output: test.fail('Not find vsock device: "%s"' % device_pattern) + device_str = None # Windows doesn't need device_str for dmesg check else: device_str = re.findall(r"%s\s%s" % (addr_pattern, device_pattern), output) if params.get("dmesg_check") == "yes": + if device_str is None: + test.log.info("Skipping dmesg check for Windows") + elif not device_str: - if not device_str: test.fail('check_vsock_cmd failed, no device "%s"' % device_pattern) else:Alternatively, restructure to only perform dmesg check for Linux:
output = session.cmd_output(check_vsock_cmd) if os_type == "windows": print("%s %s" % (device_pattern, output)) if device_pattern not in output: test.fail('Not find vsock device: "%s"' % device_pattern) else: device_str = re.findall(r"%s\s%s" % (addr_pattern, device_pattern), output) - -if params.get("dmesg_check") == "yes": - if not device_str: - test.fail('check_vsock_cmd failed, no device "%s"' % device_pattern) - else: - address = re.findall(addr_pattern, device_str[0])[0] - chk_dmesg_cmd = "dmesg" - output = re.findall(address, session.cmd_output(chk_dmesg_cmd)) - if not output: - test.fail("dmesg failed, no info related to %s" % address) - else: - error_msg = "" - for o in output: - if re.search(r"fail|error", o, re.I): - error_msg += "%s" % o - break - if error_msg: - test.fail("dmesg check failed: %s" % error_msg) + + if params.get("dmesg_check") == "yes": + if not device_str: + test.fail('check_vsock_cmd failed, no device "%s"' % device_pattern) + else: + address = re.findall(addr_pattern, device_str[0])[0] + chk_dmesg_cmd = "dmesg" + output = re.findall(address, session.cmd_output(chk_dmesg_cmd)) + if not output: + test.fail("dmesg failed, no info related to %s" % address) + else: + error_msg = "" + for o in output: + if re.search(r"fail|error", o, re.I): + error_msg += "%s" % o + break + if error_msg: + test.fail("dmesg check failed: %s" % error_msg)
🧹 Nitpick comments (1)
qemu/tests/vsock_hotplug.py (1)
99-101: Use already-definedos_typevariable for consistency.Line 100 calls
params.get("os_type")again, butos_typewas already fetched at line 34. Using the existing variable improves consistency and avoids redundant dictionary lookups.🔎 Proposed fix
finally: # For windows testing, transfer_file() will remove the file - if params.get("os_type") == "linux": + if os_type == "linux": session.cmd_output("rm -f %s" % tmp_file) session.close()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
provider/win_driver_installer_test.pyqemu/tests/cfg/virtio_win_socket_test.cfgqemu/tests/vsock_hotplug.py
🧰 Additional context used
🪛 GitHub Actions: CI
qemu/tests/vsock_hotplug.py
[error] 62-62: pylint: E0606 - Possibly using variable 'device_str' before assignment (possibly-used-before-assignment). Command: 'pylint .'
[error] 1-1: Process completed with exit code 2.
🔇 Additional comments (2)
provider/win_driver_installer_test.py (1)
32-32: LGTM! Driver list additions are consistent.The viosock driver has been properly added to all three parallel lists (driver names, HWIDs, and device names), maintaining consistency with the existing pattern.
Also applies to: 47-47, 62-62
qemu/tests/vsock_hotplug.py (1)
8-8: LGTM! Import added for Windows vsock testing.The import of
viosock_testfromprovider.win_driver_installer_testis appropriate for the Windows-specific vsock data transfer path.
leidwang
force-pushed
the
vsock_testing
branch
from
b36a2c5 to
d63e29e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qemu/tests/vsock_hotplug.py (1)
53-77: Fix:device_stris undefined whenos_type == "windows".The pipeline failure (Pylint E0606) correctly identifies that
device_stris only assigned in theelsebranch (line 59), but thedmesg_checkblock (lines 61-77) references it unconditionally. If a Windows test runs withdmesg_check == "yes", this will cause aNameError.🔎 Proposed fix
output = session.cmd_output(check_vsock_cmd) if os_type == "windows": print("%s %s" % (device_pattern, output)) if device_pattern not in output: test.fail('Not find vsock device: "%s"' % device_pattern) else: device_str = re.findall(r"%s\s%s" % (addr_pattern, device_pattern), output) - if params.get("dmesg_check") == "yes": + if params.get("dmesg_check") == "yes": - if not device_str: - test.fail('check_vsock_cmd failed, no device "%s"' % device_pattern) - else: - address = re.findall(addr_pattern, device_str[0])[0] - chk_dmesg_cmd = "dmesg" - output = re.findall(address, session.cmd_output(chk_dmesg_cmd)) - if not output: - test.fail("dmesg failed, no info related to %s" % address) + if not device_str: + test.fail('check_vsock_cmd failed, no device "%s"' % device_pattern) else: - error_msg = "" - for o in output: - if re.search(r"fail|error", o, re.I): - error_msg += "%s" % o - break - if error_msg: - test.fail("dmesg check failed: %s" % error_msg) + address = re.findall(addr_pattern, device_str[0])[0] + chk_dmesg_cmd = "dmesg" + output = re.findall(address, session.cmd_output(chk_dmesg_cmd)) + if not output: + test.fail("dmesg failed, no info related to %s" % address) + else: + error_msg = "" + for o in output: + if re.search(r"fail|error", o, re.I): + error_msg += "%s" % o + break + if error_msg: + test.fail("dmesg check failed: %s" % error_msg)This moves the
dmesg_checkblock inside the Linux (else) branch sincedmesgis a Linux-only command anddevice_stris only available there.
♻️ Duplicate comments (2)
provider/win_driver_installer_test.py (1)
470-479: Race condition: server_ready is signaled before server is actually listening.The
server_ready.set()is called beforesession.cmd_output(cmd)executes the server command. This means the client may attempt to connect before the server is actually listening, causing a connection failure. The event should be set after the server is ready, but sincesession.cmd_output()is blocking, a different synchronization approach is needed.Consider using a non-blocking command execution or adding a delay in the client, or restructuring to start the server asynchronously and detect when it's ready.
🔎 Proposed fix - add delay workaround
def server_thread(self, session, cmd): """ Execute server command in guest VM :param session: VM session object :param cmd: Server command to execute """ - self.server_ready.set() - result = session.cmd_output(cmd) - self.results["server"] = ("success", result) + try: + self.server_ready.set() + result = session.cmd_output(cmd) + self.results["server"] = ("success", result) + except Exception as e: + self.results["server"] = ("error", str(e)) def client_thread(self, cmd): """ Execute client command on host :param cmd: Client command to execute """ - self.server_ready.wait() - time.sleep(1) - result = process.system_output(cmd) - self.results["client"] = ("success", result) + try: + self.server_ready.wait() + time.sleep(3) # Increased delay to allow server startup + result = process.system_output(cmd, shell=True) + self.results["client"] = ("success", result) + except Exception as e: + self.results["client"] = ("error", str(e))Note: The exception handling aspect was also flagged in past reviews.
qemu/tests/vsock_hotplug.py (1)
98-102:tmp_fileis undefined for Windows path, causingNameError.If
os_type == "windows", the code skips lines 82-97 wheretmp_fileis defined. However, thefinallyblock referencestmp_fileon line 101. Although the condition checks for Linux, the variable reference itself occurs in all paths when Python parses the condition.Additionally, as noted in past reviews, the Windows path is missing the
simple_unplugcall fordev_vsock.🔎 Proposed fix
# Transfer data from guest to host + tmp_file = None try: if os_type == "windows": viosock_test(test, params, vm, guest_cid) + vm.devices.simple_unplug(dev_vsock, vm.monitor) else: vsock_test_tool = params["vsock_test_tool"] if vsock_test_tool == "nc_vsock": tool_bin = vsock_test.compile_nc_vsock(test, vm, session) elif vsock_test_tool == "ncat": tool_bin = path.find_command("ncat") else: raise ValueError(f"unsupported test tool: {vsock_test_tool}") tmp_file = "/tmp/vsock_file_%s" % utils_misc.generate_random_string(6) rec_session = vsock_test.send_data_from_guest_to_host( session, tool_bin, guest_cid, tmp_file ) vsock_negative_test.check_data_received(test, rec_session, tmp_file) vm.devices.simple_unplug(dev_vsock, vm.monitor) vsock_negative_test.kill_host_receive_process(test, rec_session) vsock_test.check_guest_vsock_conn_exit(test, session) finally: - # For windows testing, transfer_file() will remove the file - if params.get("os_type") == "linux": + if tmp_file: session.cmd_output("rm -f %s" % tmp_file) session.close()
🧹 Nitpick comments (2)
provider/win_driver_installer_test.py (1)
523-545: Missing cleanup of test artifacts.The function creates/copies several files but doesn't clean them up:
- Host-side: source file created by
generate_file_cmd(referenced assrc_file_path)- Guest-side: test tool copied to
C:\and the received data fileConsider adding cleanup in a
finallyblock or documenting that cleanup is the caller's responsibility.🔎 Proposed fix
def viosock_test(test, params, vm, guest_cid): """ - Trasfer data from host to VM via vsock + Transfer data from host to VM via vsock :param test: kvm test object. :param params: the dict used for parameters. :param vm: vm object. + :param guest_cid: Guest CID for vsock connection. """ session = vm.wait_for_login() - path = win_driver_utils.get_driver_inf_path( - session, test, params["virtio_win_media_type"], params["driver_name"] - ) - test_tool_src_path = path[: path.rfind("\\")] + "\\" + params["test_tool"] - session.cmd_output("xcopy %s C:\\ /y" % test_tool_src_path) - process.system_output(params["generate_file_cmd"]) - server_cmd = params["receive_data_cmd"] - client_cmd = params["send_data_cmd"] % guest_cid - - LOG_JOB.info("Transfer data from host to VM") - transfer = VSockTransfer() - transfer.run(session, server_cmd, client_cmd) - - LOG_JOB.info("Check src and dst file's md5sum") - src_output = process.system_output("md5sum %s" % params["src_file_path"]).decode() - src_md5_sum = src_output.split()[0] - dst_output = session.cmd_output(params["md5sum_check_cmd"]) - dst_md5_sum = dst_output.splitlines()[1] - if src_md5_sum != dst_md5_sum: - test.fail("File md5sum is not the same after transfer from host to guest") - else: - LOG_JOB.info("MD5 verification passed") + try: + path = win_driver_utils.get_driver_inf_path( + session, test, params["virtio_win_media_type"], params["driver_name"] + ) + test_tool_src_path = path[: path.rfind("\\")] + "\\" + params["test_tool"] + session.cmd_output("xcopy %s C:\\ /y" % test_tool_src_path) + process.system_output(params["generate_file_cmd"]) + server_cmd = params["receive_data_cmd"] + client_cmd = params["send_data_cmd"] % guest_cid + + LOG_JOB.info("Transfer data from host to VM") + transfer = VSockTransfer() + transfer.run(session, server_cmd, client_cmd) + + LOG_JOB.info("Check src and dst file's md5sum") + src_output = process.system_output("md5sum %s" % params["src_file_path"]).decode() + src_md5_sum = src_output.split()[0] + dst_output = session.cmd_output(params["md5sum_check_cmd"]) + dst_lines = dst_output.splitlines() + if len(dst_lines) < 2: + test.fail("Unexpected certutil output format: %s" % dst_output) + dst_md5_sum = dst_lines[1].strip() + if src_md5_sum != dst_md5_sum: + test.fail("File md5sum is not the same after transfer from host to guest") + else: + LOG_JOB.info("MD5 verification passed") + finally: + # Cleanup host-side generated file + process.system_output("rm -f %s" % params.get("src_file_path", ""), ignore_status=True) + session.close()qemu/tests/vsock_hotplug.py (1)
54-57: Consider using logging instead ofLine 55 uses a bare
print()statement for debug output. The rest of the test usestest.log.infofor logging. Consider replacing this with proper logging for consistency.🔎 Proposed fix
if os_type == "windows": - print("%s %s" % (device_pattern, output)) + test.log.info("Device pattern: %s, Output: %s", device_pattern, output) if device_pattern not in output: test.fail('Not find vsock device: "%s"' % device_pattern)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
provider/win_driver_installer_test.pyqemu/tests/cfg/virtio_win_socket_test.cfgqemu/tests/vsock_hotplug.py
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_win_socket_test.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
provider/win_driver_installer_test.py (3)
qemu/tests/vsock_hotplug.py (1)
run(13-103)qemu/tests/win_virtio_driver_installer_uninstall.py (1)
run(20-120)provider/win_driver_utils.py (1)
get_driver_inf_path(123-155)
🪛 GitHub Actions: CI
qemu/tests/vsock_hotplug.py
[error] 62-62: Pylint: E0606 possibly using variable 'device_str' before assignment (possibly-used-before-assignment).
🔇 Additional comments (2)
provider/win_driver_installer_test.py (1)
5-5: LGTM for viosock driver infrastructure additions.The threading import and additions to
driver_name_list,device_hwid_list, anddevice_name_listcorrectly extend the Windows driver test infrastructure for VirtIO Socket support.Also applies to: 32-32, 47-47, 62-62
qemu/tests/vsock_hotplug.py (1)
8-8: LGTM for the import.The import of
viosock_testfromprovider.win_driver_installer_testcorrectly enables Windows-specific vsock testing.
| def viosock_test(test, params, vm, guest_cid): | ||
| """ | ||
| Trasfer data from host to VM via vsock | ||
| :param test: kvm test object. | ||
| :param params: the dict used for parameters. | ||
| :param vm: vm object. | ||
| """ |
There was a problem hiding this comment.
Typo in docstring.
Line 517: "Trasfer" should be "Transfer".
🔎 Proposed fix
def viosock_test(test, params, vm, guest_cid):
"""
- Trasfer data from host to VM via vsock
+ Transfer data from host to VM via vsock
:param test: kvm test object.
:param params: the dict used for parameters.
:param vm: vm object.
+ :param guest_cid: Guest CID for vsock connection.
"""🤖 Prompt for AI Agents
In provider/win_driver_installer_test.py around lines 515 to 522, the docstring
for viosock_test contains a typo: "Trasfer" should be corrected to "Transfer";
update the docstring line to read "Transfer data from host to VM via vsock" so
the spelling is correct and documentation is accurate.
Signed-off-by: Leidong Wang <[email protected]>
leidwang
force-pushed
the
vsock_testing
branch
from
d63e29e to
c021316
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qemu/tests/vsock_hotplug.py (1)
54-69: PotentialUnboundLocalErrorwhendmesg_check=yeson Windows.
device_stris set toNoneon line 54 and only assigned in theelseblock (Linux path) on line 59. Ifos_type == "windows"anddmesg_check == "yes", line 62 will checkif not device_str:which will beTrue(sinceNoneis falsy), causing the test to fail with a misleading error message.While
dmesg_check = nois set in the config, the code should be defensive.🔎 Proposed fix
if params.get("dmesg_check") == "yes": + if os_type == "windows": + test.error("dmesg_check is not supported on Windows") if not device_str: test.fail('check_vsock_cmd failed, no device "%s"' % device_pattern)
♻️ Duplicate comments (4)
provider/win_driver_installer_test.py (3)
470-490: Add exception handling to thread methods.Both
server_threadandclient_threadlack exception handling. If either command fails, the exception will be silently lost,self.resultswill remain incomplete, and debugging failures will be difficult.Also,
process.system_output(cmd)on line 489 may needshell=Trueifsend_data_cmdcontains shell constructs like pipes.🔎 Proposed fix
def server_thread(self, session, cmd): - self.server_ready.set() - result = session.cmd_output(cmd) - self.results["server"] = ("success", result) + try: + self.server_ready.set() + result = session.cmd_output(cmd) + self.results["server"] = ("success", result) + except Exception as e: + self.results["server"] = ("error", str(e)) def client_thread(self, cmd): - self.server_ready.wait() - time.sleep(1) - result = process.system_output(cmd) - self.results["client"] = ("success", result) + try: + self.server_ready.wait() + time.sleep(1) + result = process.system_output(cmd, shell=True) + self.results["client"] = ("success", result) + except Exception as e: + self.results["client"] = ("error", str(e))
515-522: Fix typo in docstring and add missing parameter documentation.Line 517: "Trasfer" should be "Transfer". Also, the
guest_cidparameter is not documented.🔎 Proposed fix
def viosock_test(test, params, vm, guest_cid): """ - Trasfer data from host to VM via vsock + Transfer data from host to VM via vsock :param test: kvm test object. :param params: the dict used for parameters. :param vm: vm object. + :param guest_cid: Guest CID for vsock connection. """
523-545: Session not closed and transfer results not validated.
- The session opened on line 523 is never closed, leading to a resource leak.
- The
transfer.run()result is not validated—if either thread fails (once exception handling is added), the failure goes undetected.- Line 541 assumes
certutiloutput has the MD5 hash on the second line without bounds checking, risking anIndexError. It also lacks.strip()which may cause comparison failures.🔎 Proposed fix
def viosock_test(test, params, vm, guest_cid): ... session = vm.wait_for_login() - path = win_driver_utils.get_driver_inf_path( - session, test, params["virtio_win_media_type"], params["driver_name"] - ) - ... - transfer.run(session, server_cmd, client_cmd) - - LOG_JOB.info("Check src and dst file's md5sum") - src_output = process.system_output("md5sum %s" % params["src_file_path"]).decode() - src_md5_sum = src_output.split()[0] - dst_output = session.cmd_output(params["md5sum_check_cmd"]) - dst_md5_sum = dst_output.splitlines()[1] - if src_md5_sum != dst_md5_sum: - test.fail("File md5sum is not the same after transfer from host to guest") - else: - LOG_JOB.info("MD5 verification passed") + try: + path = win_driver_utils.get_driver_inf_path( + session, test, params["virtio_win_media_type"], params["driver_name"] + ) + ... + results = transfer.run(session, server_cmd, client_cmd) + # Validate transfer results + for role in ["server", "client"]: + if role not in results: + test.fail("Transfer incomplete: %s did not complete" % role) + status, output = results[role] + if status == "error": + test.fail("Transfer failed in %s: %s" % (role, output)) + + LOG_JOB.info("Check src and dst file's md5sum") + src_output = process.system_output("md5sum %s" % params["src_file_path"]).decode() + src_md5_sum = src_output.split()[0] + dst_output = session.cmd_output(params["md5sum_check_cmd"]) + dst_lines = dst_output.splitlines() + if len(dst_lines) < 2: + test.fail("Unexpected certutil output format: %s" % dst_output) + dst_md5_sum = dst_lines[1].strip() + if src_md5_sum != dst_md5_sum: + test.fail("File md5sum is not the same after transfer from host to guest") + else: + LOG_JOB.info("MD5 verification passed") + finally: + session.close()qemu/tests/vsock_hotplug.py (1)
79-101: Windows path missing device unplug and potentialUnboundLocalErrorfortmp_file.
The Windows branch (lines 80-81) only performs the data transfer but skips device unplug (line 95), leaving the vsock device plugged after the test.
tmp_fileis only defined in the Linux path (line 90), but thefinallyblock references it on line 101. If an exception occurs before line 90 in the Linux path, or if the logic changes, this will raiseUnboundLocalError.🔎 Proposed fix
+ tmp_file = None try: if os_type == "windows": viosock_test(test, params, vm, guest_cid) + vm.devices.simple_unplug(dev_vsock, vm.monitor) else: vsock_test_tool = params["vsock_test_tool"] ... tmp_file = "/tmp/vsock_file_%s" % utils_misc.generate_random_string(6) ... vm.devices.simple_unplug(dev_vsock, vm.monitor) vsock_negative_test.kill_host_receive_process(test, rec_session) vsock_test.check_guest_vsock_conn_exit(test, session) finally: - # For windows testing, transfer_file() will remove the file - if params.get("os_type") == "linux": + if tmp_file and os_type == "linux": session.cmd_output("rm -f %s" % tmp_file) session.close()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
provider/win_driver_installer_test.pyqemu/tests/cfg/virtio_win_socket_test.cfgqemu/tests/vsock_hotplug.py
🧰 Additional context used
🧬 Code graph analysis (2)
qemu/tests/vsock_hotplug.py (3)
provider/win_driver_installer_test.py (1)
viosock_test(515-545)qemu/tests/vsock_test.py (3)
compile_nc_vsock(13-41)send_data_from_guest_to_host(118-158)check_guest_vsock_conn_exit(161-178)qemu/tests/vsock_negative_test.py (2)
check_data_received(11-26)kill_host_receive_process(30-40)
provider/win_driver_installer_test.py (1)
provider/win_driver_utils.py (1)
get_driver_inf_path(123-155)
🔇 Additional comments (3)
qemu/tests/cfg/virtio_win_socket_test.cfg (1)
1-27: Configuration looks reasonable for Windows vsock hotplug testing.The configuration correctly sets up Windows-specific vsock test parameters including tool paths, data transfer commands, and hotplug variant. The
check_vsock_cmdnow outputs all present devices, and thedevice_patternis used for matching.provider/win_driver_installer_test.py (1)
32-32: LGTM! Driver list entries are correctly added.The
viosockdriver, its hardware IDs, and device name are properly added in parallel to the existing lists.Also applies to: 47-47, 62-62
qemu/tests/vsock_hotplug.py (1)
8-8: Import added for Windows vsock test support.The import of
viosock_testfromprovider.win_driver_installer_testcorrectly enables Windows-specific vsock handling.
1 participant
ID:4796
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.