win_guest_debugging_tool: Optimize and simplify script #4417
Conversation
WalkthroughReplaces the previous multi-stage BaseVirtTest flow in qemu/tests/win_guest_debugging_tool.py with a single WinDebugToolTest class exposing an execute() entry point. Removes lifecycle hooks (initialize/setup/run_once/before_run_once/after_run_once/cleanup). Introduces centralized session management ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
🧹 Nitpick comments (1)
qemu/tests/win_guest_debugging_tool.py (1)
112-124: Consider more precise file matching.Line 120 checks if filenames are substrings of the entire output, which could cause false positives if one filename contains another as a substring.
Apply this diff for more accurate matching:
- missing_files = [f for f in target_files if f not in output] + output_files = output.splitlines() + missing_files = [f for f in target_files if f not in output_files]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
qemu/tests/win_guest_debugging_tool.py
[error] 141-141: Pylint: E1102 'test_method' is not callable (not-callable). Command failed: pylint .
🔇 Additional comments (14)
qemu/tests/win_guest_debugging_tool.py (14)
30-44: LGTM!The session management logic correctly tracks all created sessions for cleanup and provides flexibility to reuse or create new sessions as needed.
46-54: LGTM!The cleanup logic safely handles session closure with proper error handling and logging.
56-73: LGTM!The setup flow correctly initializes the VM, locates the script, and prepares the working directory. The
str()conversion on line 66 is redundant but harmless.
144-153: LGTM!Basic validation logic is clear and correct.
155-180: LGTM!ZIP validation logic correctly extracts and compares folder sizes to ensure packaging integrity.
182-195: LGTM!The stability test correctly runs multiple iterations. Consider verifying cleanup success if intermittent failures occur, but current approach is acceptable.
260-333: LGTM!Registry collection test logic is comprehensive. The nested helper functions and try-finally cleanup are well-structured. Note that parsing on lines 280 and 284 assumes specific output formats and will raise
ValueErrorif format changes.
335-362: LGTM!The BSOD test correctly triggers NMI, waits for crash/reboot, and verifies dump collection. Session management after reboot is handled properly.
364-429: LGTM!The driver and system info collection test is well-structured with appropriate defensive checks for driver existence and proper handling of system reboots.
431-499: LGTM!Network adapter test handles connection drops gracefully during adapter configuration changes. The timeout handling and session re-establishment logic is appropriate for this scenario.
545-608: LGTM!IO metrics test correctly establishes baseline, generates load with proper threading and cleanup, and verifies metrics increased. The walrus operator usage is valid for Python 3.8+.
610-628: LGTM!The execution flow and entry point are well-structured. The try-finally block ensures proper cleanup of all tracked sessions regardless of test outcome.
536-536: Verify Windows copy command compatibility.Line 536 uses
cpwhich is a Unix command. On Windows systems using cmd.exe, this will fail becausecopyis the correct command. If the session context is PowerShell,cpworks as an alias forCopy-Item. Verify the session type and consider using a platform-agnostic approach or ensure PowerShell is the intended shell.
27-27: Remove unusedtmp_dirinitialization or fix if intended for use.The
self.tmp_dirvariable at line 27 is never used in this file. Either remove it, or if needed, fix the initialization:params.get()does not automatically expand${tmp_dir}. Use explicit template expansion (via virttest helpers) or calldata_dir.get_tmp_dir()instead.Likely an incorrect or invalid review comment.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
65243c4 to
7fce07d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
qemu/tests/win_guest_debugging_tool.py (7)
71-73: Consider checking directory creation and navigation.The code creates a temp directory and changes to it without verifying success. If the directory creation or
cdcommand fails, subsequent commands may execute in the wrong location.Apply this diff to add error checking:
- error_context.context("Creating temporary work directory.", LOG_JOB.info) - self.session.cmd_output(self.params["cmd_create_dir"] % self.tmp_dir) - self.session.cmd(f"cd {self.tmp_dir}") + error_context.context("Creating temporary work directory.", LOG_JOB.info) + status, _ = self.session.cmd_status_output(self.params["cmd_create_dir"] % self.tmp_dir) + if status != 0: + self.test.error(f"Failed to create temporary directory: {self.tmp_dir}") + status, _ = self.session.cmd_status_output(f"cd {self.tmp_dir}") + if status != 0: + self.test.error(f"Failed to change to directory: {self.tmp_dir}")
114-126: Consider checking command execution status.The
dir /bcommand could fail (e.g., if the path doesn't exist), but the code doesn't verify the command succeeded before parsing output.Apply this diff to add error checking:
def _check_generated_files(self, path, is_dump=False): """ Verifies that the expected files are generated in the given path. """ - output = str(self.session.cmd_output(f"dir /b {path}")).strip() + status, output = self.session.cmd_status_output(f"dir /b {path}") + if status != 0: + self.test.error(f"Failed to list files in '{path}': command returned status {status}") + output = output.strip()
279-289: Add error handling for registry value parsing.Lines 285 and 289 parse integers from command output without error handling. If the output format is unexpected, this will raise an unhandled
ValueErrororIndexError.Apply this diff to add error handling:
def get_reg_value_from_file(file, item, subkey): cmd = self.params["cmd_findstr_in_file"] % ( file, f"{item}\\\\{subkey}", ) output = self.session.cmd_output(cmd) - return int(output.split(":")[-1].strip()) + try: + return int(output.split(":")[-1].strip()) + except (ValueError, IndexError) as e: + self.test.error(f"Failed to parse registry value from file: {e}. Output: {output}") def get_reg_value_from_guest(item, subkey): cmd = self.params["cmd_reg_query"] % (item, subkey) - return int(self.session.cmd_output(cmd)) + try: + return int(self.session.cmd_output(cmd)) + except ValueError as e: + self.test.error(f"Failed to parse registry value from guest: {e}")
376-390: Add error handling for system name parsing.Lines 384-386 parse the system name by splitting on ":" without verifying the output format. If the command output is unexpected, this could raise an
IndexError.Apply this diff to add error handling:
msinfo_file = f"{paths['log_folder']}\\msinfo32.txt" -file_sysname = ( - self.session.cmd_output( - self.params["cmd_query_from_file"] % (msinfo_file, "System Name") - ) - .split(":")[1] - .strip() -) +output = self.session.cmd_output( + self.params["cmd_query_from_file"] % (msinfo_file, "System Name") +) +parts = output.split(":") +if len(parts) < 2: + self.test.error(f"Unexpected format when querying system name: {output}") +file_sysname = parts[1].strip()
473-476: Add error handling for Jumbo Packet value parsing.Line 476 parses an integer from the command output without error handling. If the output format is unexpected, this will raise an unhandled
ValueError.Apply this diff:
jp_from_file_raw = self.session.cmd_output( self.params["cmd_findstr_in_file"] % (network_file, "Jumbo Packet") ) -jp_from_file = int(jp_from_file_raw.split()[-1]) +try: + jp_from_file = int(jp_from_file_raw.split()[-1]) +except (ValueError, IndexError) as e: + self.test.error(f"Failed to parse Jumbo Packet value: {e}. Output: {jp_from_file_raw}")
540-548: Check command execution status.Line 543 runs the command from documentation without checking its exit status. If the command fails, the test should detect it before checking the output.
Apply this diff:
# Copy script to tmp_dir to run the example command self.session.cmd(f"cp {self.script_path} .") # The example from the docs is the command to run -_, output = self.session.cmd_status_output(command_line, timeout=360) +status, output = self.session.cmd_status_output(command_line, timeout=360) +if status != 0: + self.test.fail(f"Command from documentation failed with status {status}")
566-570: Complex dict comprehension may be difficult to maintain.Lines 567-570 use a dense dict comprehension with walrus operators and inline conditions. While functional, this may be harder to debug if metric parsing fails.
Consider a more explicit approach for better maintainability:
-baseline_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in baseline_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) -} +baseline_metrics = {} +for line in baseline_output.splitlines(): + if "Disk" in line: + parts = line.split(":")[-2:] + if len(parts) >= 2: + try: + key = parts[0] + value = float(parts[1].split()[0].replace(",", "")) + baseline_metrics[key] = value + except (ValueError, IndexError) as e: + LOG_JOB.warning("Failed to parse IO metric line: %s. Error: %s", line, e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (2)
provider/storage_benchmark.py (1)
session(83-90)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (11)
qemu/tests/win_guest_debugging_tool.py (11)
1-29: LGTM!The imports are appropriate, and the class initialization is well-structured with clear instance variables for session tracking.
46-54: LGTM!The cleanup logic properly handles exceptions during session closure and ensures the session list is cleared.
75-112: LGTM!The script execution and path parsing logic is well-structured. The exit status check (line 87-88) properly addresses the past review concern, ensuring failures are detected early.
128-144: LGTM!The dynamic dispatch logic correctly uses
hasattr(line 138) beforegetattr(line 140), addressing the previous pylint concern.
146-155: LGTM!The basic script execution test is straightforward and correctly validates the expected outputs.
185-197: LGTM!The stability test appropriately runs the script multiple times and cleans up after each iteration.
200-263: LGTM!The user-friendliness test comprehensively covers invalid parameter handling and interruption scenarios. The empty output check (lines 247-252) properly addresses the past review concern.
341-367: LGTM!The sensitive data collection test properly triggers BSOD, handles reboot, and verifies dump file collection.
437-504: Good network adapter test with proper timeout handling.The test correctly handles
ShellTimeoutErrorduring adapter configuration changes (lines 456-462, 494-498) by re-establishing the session. This is a good pattern for network configuration changes that may drop connectivity temporarily.
551-613: LGTM on IO limits test overall!The IO limits test has good structure with baseline collection, load generation in a separate thread, and proper cleanup using
stop_eventandfinallyblock.
615-633: LGTM!The
execute()method andrun()entry point are clean and follow the expected test framework pattern. Thefinallyblock ensures sessions are cleaned up even if tests fail.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
7fce07d to
16fedb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
qemu/tests/win_guest_debugging_tool.py (6)
85-92: Include script output in failure message for easier debugging
_run_script_and_get_paths()now correctly checks the exit status, but when the script fails you lose all context about why. Including at least the captured output in the failure message will make debugging much easier:- status, output = self.session.cmd_status_output(cmd, timeout=720) - if status != 0: - self.test.fail(f"Script execution failed with status {status}") + status, output = self.session.cmd_status_output(cmd, timeout=720) + if status != 0: + self.test.fail( + f"Script execution failed with status {status}.\nOutput:\n{output}" + )Given this is a test-only path, the extra log volume should be acceptable.
117-129: Minor robustness tweak for_check_generated_filesThe structure here is good: you centralize file presence checks keyed by
is_dump. Once thedircommand is quoted as suggested earlier, the remaining logic is straightforward. If you want to harden it slightly, consider splittingtarget_fileswith.split(",")and normalizing with.strip()to tolerate spaces in the config (e.g."a.txt, b.txt"), but the current approach is acceptable if configs are well-formed.
197-277: User-friendliness and interruption handling logic looks good overallThe multi-iteration stability check and the user-friendliness test are well-structured:
- Running the script 5 times and cleaning up each
log_folder/ZIP exercises basic robustness.- The invalid-parameter loop covers multiple bad args and checks for a friendly prompt.
- The interruption path uses a second session and a thread to kill the PowerShell process while treating
ShellTimeoutErroras expected, then verifies the presence of a signal file and a successful recovery run.Two minor, optional ideas:
- If
cmd_remove_diris truly directory-only, consider using a separate remove-file command for the ZIP to avoid silent failures.- In
run_and_interrupt(), you might logoutputwhen a non-timeout exception occurs to aid debugging.Current behavior is still acceptable for a test.
383-448: Driver/system info collection test thoroughly exercises dynamic changes
_check_trigger_driver_msinfo_collection()does a nice job of:
- Comparing system name between live guest output and
msinfo32.txt(case-insensitive).- Changing the system name and uninstalling a target driver (with a graceful warning if no OEM INF is found).
- Rebooting, re-running the script, and verifying the new system name is captured.
- Checking the driver list to ensure the uninstalled driver is no longer present.
The only minor fragility is reliance on
split(":")[1]when parsingcmd_query_from_fileoutput; if the format ever drops the colon, you’ll getIndexError. If that’s under your control, current code is fine; otherwise you might want a small guard there. Not required, just something to keep in mind.
520-562: Add explicit status check when running documentation example commandThe documentation variant is a nice touch (checking for standard docs, parsing a PowerShell example from README, then actually running it). When you execute the example:
_, output = self.session.cmd_status_output(command_line, timeout=360)you ignore the exit status and only rely on later regex matching. It’s safer and more transparent to fail immediately on a non‑zero status:
- _, output = self.session.cmd_status_output(command_line, timeout=360) + status, output = self.session.cmd_status_output(command_line, timeout=360) + if status != 0: + self.test.fail( + f"Documentation example command failed with status {status}." + )You’ll still keep the existing
Log folder pathcheck as an additional guard, but status-based failure will give a clearer signal when the example itself is broken.
564-627: Harden IO metrics parsing to avoid future format‑driven crashesThe IO limits test is well thought out: you collect a baseline, generate load in a background session/thread, re-run the script, and then assert that at least one disk metric increases.
The parsing logic, however, assumes that for any line containing
"Disk":m = line.split(":")[-2:] m[0], m[1] # label and "value ..." part
mwill always have two elements. If the output format changes (e.g. no colon on a “Disk” line), this will raiseIndexErrorand crash the test rather than producing a clear failure.A slightly more defensive (yet still simple) parsing loop would make this more future‑proof:
- baseline_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in baseline_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + baseline_metrics = {} + for line in baseline_output.splitlines(): + if "Disk" not in line or ":" not in line: + continue + label, value_part = line.split(":", 1) + try: + value = float(value_part.split()[0].replace(",", "")) + except ValueError: + continue + baseline_metrics[label.strip()] = value @@ - new_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in new_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + new_metrics = {} + for line in new_output.splitlines(): + if "Disk" not in line or ":" not in line: + continue + label, value_part = line.split(":", 1) + try: + value = float(value_part.split()[0].replace(",", "")) + except ValueError: + continue + new_metrics[label.strip()] = valueThis keeps the intent the same but avoids brittle assumptions about the exact colon structure of each line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qemu/tests/win_guest_debugging_tool.py(1 hunks)
🔇 Additional comments (5)
qemu/tests/win_guest_debugging_tool.py (5)
12-28: Refactored single-class structure and centralized execution flow look solidThe move to a single
WinDebugToolTestwith_get_session()/_cleanup_sessions()andexecute()wrappingsetup()+_execute_test_variant()is much clearer than the prior multi-stage lifecycle. Dynamic dispatch via_execute_test_variant()keeps variant logic contained while still explicit, and_open_sessionstracking plus final cleanup should prevent session leaks, including for extranew=Truesessions. This fits the stated PR goals of simplification and better session management.Also applies to: 30-58, 131-147, 628-646
149-196: ZIP verification logic is clear and numerically robust
_check_script_execution()and_check_zip_package()make good use of_run_script_and_get_paths()and the size check fix:
- You now parse folder sizes as integers with
replace(",", ""), which avoids fragile string equality and matches the earlier review feedback.- The error path when parsing fails (
ValueError) is explicit and includes both raw strings, which will help diagnose environment/locale issues.Once the path-quoting change is applied to upstream helpers, this whole flow looks solid.
279-352: Disk/registry collection test has a clear baseline‑modify‑verify‑cleanup pattern
_check_disk_registry_collection()is nicely structured:
- Takes a baseline from both the collected file and live registry and compares them.
- Modifies existing and new keys, re-runs the script, and verifies each expected change is captured.
- Uses a
finallyblock to ensure registry cleanup and restoration of the original value regardless of failures.Given the strong dependency on config commands, the integer conversions and comparisons are reasonable, and the explicit cleanup is especially welcome in a stateful Windows guest.
354-381: Sensitive data / BSOD collection flow is coherentThe
_check_includeSensitiveData_collection()variant:
- Triggers a BSOD via
nmi(), waits, reboots via the configured method, and re-establishes a session.- Verifies that dump artifacts exist before even invoking the script.
- Then runs with
-IncludeSensitiveDataand asserts bothdump_folderanddump_zipare present.This end-to-end flow (crash → reboot → verify dumps → run script → verify collected dumps) is a good coverage of the sensitive-data path.
450-518: Network adapter Jumbo Packet test covers modify‑verify‑restore well
_check_networkadapter_collection():
- Captures the current Jumbo Packet value, applies a new value, and tolerates
ShellTimeoutErrorby re-establishing the session.- Verifies the new value both via the adapter query and in the collected
NetworkInterfaces.txt.- Restores the original value in a
try/exceptblock, again handling timeouts by refreshing the session and then confirming the final value.This gives good coverage of both the guest configuration and what the tool actually records, and the cleanup logic avoids leaving the guest in a modified state.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
16fedb5 to
afd9e4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
qemu/tests/win_guest_debugging_tool.py (1)
76-76: Quote the path to handle spaces.The
cdcommand uses an unquoted path. Ifself.tmp_dircontains spaces (common on Windows, e.g., underC:\Program Files), the command will fail.Apply this diff:
- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')This same issue appears in multiple other locations throughout the file (lines 87, 121, 225, 241, 267, 364, 426, 475, 527, 554, etc.). Consider applying the same fix consistently across all path interpolations.
🧹 Nitpick comments (2)
qemu/tests/win_guest_debugging_tool.py (2)
538-561: Consider validating the extracted command before execution.The code extracts a PowerShell command from README.md and executes it (line 556). While this is appropriate for testing documentation accuracy, consider adding a basic sanity check (e.g., verifying it starts with expected patterns or doesn't contain suspicious characters) to prevent issues if the documentation is malformed.
579-583: Consider extracting the metrics parsing logic into a helper method.The dict comprehension with the walrus operator is functional but dense. For better readability and maintainability, consider extracting this pattern into a helper method since it's repeated twice (here and at lines 618-622).
Example refactor:
def _parse_io_metrics(self, output): """Parse IO metrics from command output.""" metrics = {} for line in output.splitlines(): if "Disk" in line: parts = line.split(":")[-2:] if len(parts) == 2: key = parts[0] value = float(parts[1].split()[0].replace(",", "")) metrics[key] = value return metricsThen use:
baseline_metrics = self._parse_io_metrics(baseline_output)Also applies to: 618-622
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (2)
provider/storage_benchmark.py (1)
session(83-90)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (1)
qemu/tests/win_guest_debugging_tool.py (1)
629-637: LGTM! Clean execution flow with proper cleanup.The execute method properly orchestrates setup, test execution, and cleanup using try/finally, ensuring sessions are always cleaned up regardless of test outcome.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
2 times, most recently
from
e6eca6c to
0a5566e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
qemu/tests/win_guest_debugging_tool.py (5)
69-76: Quote paths to handle spaces.This is a continuation of the previously flagged issue. Lines 69 and 76 interpolate paths without quoting, which will break if paths contain spaces (common on Windows, e.g.,
C:\Program Files).- self.script_path = str(self.session.cmd_output(cmd, timeout=360)).strip() + self.script_path = str(self.session.cmd_output(cmd, timeout=360)).strip().strip('"') ... - self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
85-91: Quote script path in PowerShell command.The script path is not quoted, which will fail for paths with spaces. This was previously flagged.
cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {extra_args}" + f'-File "{self.script_path}" {extra_args}' )
121-121: Quote path in dir command.- output = str(self.session.cmd_output(f"dir /b {path}")).strip() + output = str(self.session.cmd_output(f'dir /b "{path}"')).strip()
223-226: Quote script path in commands.cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" + f'-File "{self.script_path}" {param}' ) ... - cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = f'powershell.exe -ExecutionPolicy Bypass -File "{self.script_path}"'Also applies to: 241-241
464-464: Quote path in dir command.- dir_output = self.session.cmd_output(f"dir {self.script_dir}") + dir_output = self.session.cmd_output(f'dir "{self.script_dir}"')
🧹 Nitpick comments (4)
qemu/tests/win_guest_debugging_tool.py (4)
250-271: Add timeout to thread join to prevent indefinite hang.Line 271
script_thread.join()has no timeout. If the thread hangs (e.g., due to unexpected session behavior), the test will block indefinitely. The IO test at line 663 correctly uses a timeout.- script_thread.join() + script_thread.join(timeout=30) + if script_thread.is_alive(): + LOG_JOB.warning("Script thread did not terminate within timeout")
382-384: Consider wrapping int() conversions with error handling.Multiple
int()calls on command output (lines 382-384, 402-404, 450-452) could raiseValueErrorif the output is unexpected. While such failures indicate real issues, wrapping with error handling would provide clearer diagnostics.- original_jp_value = int( - self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) - ) + jp_output = self.session.cmd_output( + self.params["check_adapter_jp_info"] % adapter_name + ).strip() + try: + original_jp_value = int(jp_output) + except ValueError: + self.test.error(f"Failed to parse Jumbo Packet value: '{jp_output}'")
526-536: Helper functions could benefit from error handling.The nested helper functions
get_reg_value_from_fileandget_reg_value_from_guestcallint()on command output without error handling. If registry queries fail or return unexpected output, the error message won't be descriptive.
605-605: Quote paths in dir command.The
dmp_fileandminidmp_folderpaths should be quoted to handle spaces.- output = self.session.cmd_output(f"dir {dmp_file} && dir {minidmp_folder}") + output = self.session.cmd_output(f'dir "{dmp_file}" && dir "{minidmp_folder}"')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(1 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/win_guest_debugging_tool.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (3)
provider/storage_benchmark.py (1)
session(83-90)provider/virt_storage/storage_volume.py (1)
info(202-217)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (6)
qemu/tests/win_guest_debugging_tool.py (6)
12-28: Clean class initialization structure.The refactored
WinDebugToolTestclass with its instance variables and session tracking list is well-organized. The consolidation from a multi-class inheritance to a single self-contained class improves maintainability.
30-47: Session tracking is now robust.The method correctly ensures both reused and newly created sessions are tracked in
_open_sessions, addressing the previous tracking gap.
131-147: LGTM!The dispatcher pattern using
hasattrfollowed bygetattrcleanly resolves the previous pylint issue while maintaining extensibility for new test variants.
160-195: Folder size comparison is now robust.The integer parsing with
try/except ValueErrorand comma removal properly handles locale-specific formatting, addressing the previous concern.
681-690: LGTM!The
executemethod properly usestry/finallyto ensure sessions are cleaned up regardless of test outcome.
693-700: LGTM!The entry point is clean and self-documenting.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
0a5566e to
71ec599
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
qemu/tests/win_guest_debugging_tool.py (4)
78-115: Unquoted Windows paths in commands can break when paths contain spacesSeveral commands interpolate paths without quotes, which will fail or misparse when any directory contains spaces (e.g. under
C:\Program Filesor a user profile directory). Examples:
-File {self.script_path}in_run_script_and_get_paths()and_check_user_friendliness().cd {self.tmp_dir}in multiple places.dir /b {path}anddir {log_path}/dir {self.script_dir}.dir {dmp_file} && dir {minidmp_folder}.- Converting README command to
full_commandwith-File {ps_script_and_args}.Please consistently quote interpolated paths. For instance:
@@ def _run_script_and_get_paths(self, extra_args=""): - cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {extra_args}" - ) + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}" {extra_args}' + ) @@ def _check_user_friendliness(self): - cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" - ) + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}" {param}' + ) @@ def _check_user_friendliness(self): - cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = ( + 'powershell.exe -ExecutionPolicy Bypass ' + f'-File "{self.script_path}"' + ) @@ def _check_generated_files(self, path, is_dump=False): - output = str(self.session.cmd_output(f"dir /b {path}")).strip() + output = str(self.session.cmd_output(f'dir /b "{path}"')).strip() @@ def _check_user_friendliness(self): - dir_output = self.session.cmd_output(f"dir {log_path}") + dir_output = self.session.cmd_output(f'dir "{log_path}"') @@ def _check_documentation(self): - dir_output = self.session.cmd_output(f"dir {self.script_dir}") + dir_output = self.session.cmd_output(f'dir "{self.script_dir}"') @@ def _check_documentation(self): - full_command = ( - f"powershell.exe -ExecutionPolicy Bypass -File {ps_script_and_args}" - ) + full_command = ( + f'powershell.exe -ExecutionPolicy Bypass ' + f'-File "{ps_script_and_args.split()[0]}" ' + f'{" ".join(ps_script_and_args.split()[1:])}' + ) @@ def _check_includeSensitiveData_collection(self): - output = self.session.cmd_output(f"dir {dmp_file} && dir {minidmp_folder}") + output = self.session.cmd_output( + f'dir "{dmp_file}" && dir "{minidmp_folder}"' + )(Adjust the README conversion logic as needed to match the actual expected format.)
This will make the tests resilient regardless of where the script/logs reside on the guest.
Also applies to: 213-227, 238-243, 275-281, 303-312, 316-323, 329-330, 396-397, 431-436, 488-499, 531-537, 565-568, 575-578, 586-593, 686-690
671-698: Consider quoting dump paths and tightening the “dmp” checkTwo small points:
dir {dmp_file} && dir {minidmp_folder}should quote both paths to be robust to spaces (see earlier path-quoting comment).- Checking
if "dmp" not in output.lower()might give false positives if the word appears in an error message rather than an actual file listing. If you run into flakes, consider parsingdiroutput lines and explicitly looking for*.dmpentries.Current implementation is acceptable but could be hardened in those ways.
700-759: Dictionary comprehensions for IO metrics can still raise IndexError on malformed linesIn both
baseline_metricsandnew_metrics, the comprehension uses:m[0]: float(m[1].split()[0].replace(",", "")) for line in ... if "Disk" in line and (m := line.split(":")[-2:])If a line contains
"Disk"but no colon,line.split(":")[-2:]yields a single-element list andm[1]raisesIndexError. This is exactly the scenario flagged in an earlier review.You can harden this by checking
len(m) >= 2before accessing:- baseline_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in baseline_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + baseline_metrics = { + m[0]: float(m[1].split()[0].replace(",", "")) + for line in baseline_output.splitlines() + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 + } @@ - new_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in new_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + new_metrics = { + m[0]: float(m[1].split()[0].replace(",", "")) + for line in new_output.splitlines() + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 + }Optionally, you could log or skip lines where parsing fails to make the test more debuggable.
59-77: Consider quotingtmp_dirwhen changing directories
cd {self.tmp_dir}will break iftmp_dircontains spaces (very common on Windows). It’s safer to quote the path:- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')This also applies to other
cd {self.tmp_dir}calls later in the file.
🧹 Nitpick comments (3)
qemu/tests/win_guest_debugging_tool.py (3)
100-115: Tighten file presence check by matching filenames instead of substringsCurrently
missing_files = [f for f in target_files if f not in output]relies on substring search over the rawdiroutput, which can give false positives if filenames are substrings of others.You can make it more robust by splitting into lines:
- output = str(self.session.cmd_output(f'dir /b "{path}"')).strip() - target_files = self.params[target_files_key].split(",") - - missing_files = [f for f in target_files if f not in output] + output = str(self.session.cmd_output(f'dir /b "{path}"')).strip() + existing = {line.strip() for line in output.splitlines() if line.strip()} + target_files = [f.strip() for f in self.params[target_files_key].split(",")] + + missing_files = [f for f in target_files if f not in existing]This keeps behavior clear and avoids accidental matches.
Also applies to: 117-130
197-211: Multi-run stability loop is fine; optional cleanup enhancementRunning the script 5 times with per-iteration cleanup of the log folder and zip is reasonable.
If you later see flakiness from partial cleanup (e.g., other artifacts left by the script), consider delegating cleanup to a helper that removes all script-generated directories under
tmp_dir, but this is not strictly necessary right now.
427-521: Network adapter “Jumbo Packet” test is good; minor edge-case noteThe flow of baseline → change setting (with timeout handling) → verify via script output → restore original value is solid and well logged.
One edge case: if
cmd_findstr_in_filereturns multiple matching lines, only the first is used; that’s probably acceptable here, but if the output format ever duplicates “Jumbo Packet” lines for the same adapter, you might want to filter more specifically (e.g., by adapter name).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(1 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/win_guest_debugging_tool.cfg
🔇 Additional comments (12)
qemu/tests/win_guest_debugging_tool.py (12)
18-47: Session lifecycle design looks good and covers reuse/cleanup correctlyThe
_get_session/_cleanup_sessionspattern plus settingself.sessioninsetup()gives you predictable reuse and ensures all sessions are tracked for cleanup. Reusingself.sessionwhennew=Falsewhile still tracking it in_open_sessionsis a nice balance between efficiency and safety.
78-92: Good centralization of script execution and exit-status handling
_run_script_and_get_paths()nicely encapsulates PowerShell execution, checks the exit status early, and gives a single place to adjust timeouts / arguments. That should make future variant additions much easier.
131-147: Dynamic variant dispatch is clean and explicit
_execute_test_variant()’swindegtool_check_typedispatcher with thehasattrcheck gives a straightforward mapping from config to_check_*methods and fixes the earlier pylint issue.
149-159: Script execution variant is straightforward and robustThe basic execution check builds directly on
_run_script_and_get_paths()and fails fast if the expected log artifacts are missing. No further issues here.
161-195: ZIP validation logic looks solidParsing folder sizes into integers and failing on parse errors is a good improvement over raw string comparison. This should be resilient to whitespace/format differences as long as the command outputs plain numbers.
213-331: User-friendliness / interruption test is thoughtfully structuredThe invalid-parameter loop, threaded interruption with
ShellTimeoutErrorhandling, and the new PowerShell-basedSystemInfo_*discovery with a fallback all look well-considered. The extra logging around the directory detection and constructed path should help a lot in debugging intermittent guest behavior.
333-425: Driver / msinfo collection scenario is well-coveredThe baseline-vs-file system name comparison using cleaned alphanumeric tokens plus the reboot + re-collection flow is nicely robust. The defensive logging around which line is used from
msinfo32.txtshould make mismatches diagnosable, and the driver uninstall + verification steps are clear.
523-594: Documentation check is comprehensive and nicely aligned with READMEVerifying presence of standard docs, extracting the
.ps1example from a fenced PowerShell block, converting it into an executable command, and then asserting that it produces a log path gives strong coverage of “docs match reality”. The logging around which command was found and how it was converted looks very helpful.
596-669: Registry collection and cleanup flow looks correctUsing helpers to read registry values from both the collected file and the live guest, then modifying keys, re-running the script, and finally cleaning up in a
finallyblock is exactly the right pattern here. The mismatch messages are specific enough to quickly pinpoint which part failed.
704-747: IO load test structure and cleanup are solidThe baseline/new metrics collection, background IO thread with a
stop_event, andtry/finallyensuring the thread is stopped and joined are all well-structured. The finalany(new_metrics.get(k, 0) > v ...)check nicely captures the “load increased at least one metric” condition.Also applies to: 749-763
765-773: execute() wrapper correctly centralizes setup/teardownWrapping
setup()/_execute_test_variant()in atry/finallywith_cleanup_sessions()ensures sessions are always closed, even on early failures, which was one of the main refactor goals.
776-783: run() entrypoint cleanly bridges to the new test classReplacing the old BaseVirtTest lifecycle with a simple instantiation and
execute()call keeps the public API the same for the framework while isolating all logic inWinDebugToolTest. No issues here.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
71ec599 to
df6c911
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
qemu/tests/cfg/win_guest_debugging_tool.cfg (1)
94-100: Quote%splaceholders incmd_cp_fileso scripts in paths with spaces copy correctly.
cmd_cp_filecurrently expands to:cmd_cp_file = powershell.exe -Command "cp %s %s"The Python test calls:
self.session.cmd(self.params["cmd_cp_file"] % (self.script_path, "."))If
self.script_pathcontains spaces, this becomes e.g.cp C:\Program Files\CollectSystemInfo.ps1 ., which PowerShell will misparse as two arguments. To make this robust, quote the placeholders inside the PowerShell command:- cmd_cp_file = powershell.exe -Command "cp %s %s" + cmd_cp_file = powershell.exe -Command "cp '%s' '%s'"This keeps formatting flexible while ensuring both source and destination paths are safe, regardless of spaces.
qemu/tests/win_guest_debugging_tool.py (3)
78-92: Quoteself.script_pathin PowerShell invocations to handle spaces safely.All three script executions interpolate
self.script_pathunquoted:
_run_script_and_get_paths()(Lines 85–88)- Invalid‑parameter loop in
_check_user_friendliness()(Lines 223–226)run_and_interrupt()in_check_user_friendliness()(Line 241)If the script lives under a path with spaces (e.g.
C:\Program Files\...), PowerShell will misparse arguments and the test will fail even though the tool is fine.Consider updating these calls as follows:
@@ - cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {extra_args}" - ) + cmd = ( + 'powershell.exe -ExecutionPolicy Bypass ' + f'-File "{self.script_path}" {extra_args}' + ) @@ - for param in invalid_params: - cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" - ) + for param in invalid_params: + cmd = ( + 'powershell.exe -ExecutionPolicy Bypass ' + f'-File "{self.script_path}" {param}' + ) @@ - cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = ( + 'powershell.exe -ExecutionPolicy Bypass ' + f'-File "{self.script_path}"' + )(Optionally, you could also include
outputin the failure message in_run_script_and_get_paths()to ease debugging when the script fails.)Also applies to: 223-226, 238-243
59-77: Quote guest paths incd/dircommands to avoid breakage on paths with spaces.Several shell commands interpolate directory/file paths without quotes:
cd {self.tmp_dir}insetup(),_check_trigger_driver_msinfo_collection(),_check_networkadapter_collection(),_check_includeSensitiveData_collection().dir /b {path}in_check_generated_files().dir {log_path}in_check_user_friendliness().dir {self.script_dir}in_check_documentation().dir {dmp_file} && dir {minidmp_folder}in_check_includeSensitiveData_collection().If any of these paths contain spaces (quite possible for
%TEMP%, script directories, or dump locations), the commands will fail or misinterpret arguments.Updating them to consistently quote the interpolated paths would make the tests robust:
@@ - self.session.cmd_output(self.params["cmd_create_dir"] % self.tmp_dir) - self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd_output(self.params["cmd_create_dir"] % self.tmp_dir) + self.session.cmd(f'cd "{self.tmp_dir}"') @@ - output = str(self.session.cmd_output(f"dir /b {path}")).strip() + output = str(self.session.cmd_output(f'dir /b "{path}"')).strip() @@ - dir_output = self.session.cmd_output(f"dir {log_path}") + dir_output = self.session.cmd_output(f'dir "{log_path}"') @@ - self.session.cmd_output(self.params["reboot_command"]) - self.session = self._get_session(new=True) - self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd_output(self.params["reboot_command"]) + self.session = self._get_session(new=True) + self.session.cmd(f'cd "{self.tmp_dir}"') @@ - self.session = self._get_session(new=True) - self.session.cmd(f"cd {self.tmp_dir}") + self.session = self._get_session(new=True) + self.session.cmd(f'cd "{self.tmp_dir}"') @@ - dir_output = self.session.cmd_output(f"dir {self.script_dir}") + dir_output = self.session.cmd_output(f'dir "{self.script_dir}"') @@ - self.vm.reboot(self.session, method=self.params["reboot_method"]) - self.session = self._get_session(new=True) - self.session.cmd(f"cd {self.tmp_dir}") + self.vm.reboot(self.session, method=self.params["reboot_method"]) + self.session = self._get_session(new=True) + self.session.cmd(f'cd "{self.tmp_dir}"') @@ - dmp_file = self.params["memory_dmp_file"] - minidmp_folder = self.params["mini_dmp_folder"] - output = self.session.cmd_output(f"dir {dmp_file} && dir {minidmp_folder}") + dmp_file = self.params["memory_dmp_file"] + minidmp_folder = self.params["mini_dmp_folder"] + output = self.session.cmd_output( + f'dir "{dmp_file}" && dir "{minidmp_folder}"' + )This addresses a broad class of fragile path‑handling issues in one place.
Also applies to: 117-123, 297-303, 368-371, 416-423, 481-490, 619-621, 626-630
652-659: Harden IO metrics parsing to avoidIndexError/ValueErroron unexpected lines.Both
baseline_metricsandnew_metricsuse:baseline_metrics = { m[0]: float(m[1].split()[0].replace(",", "")) for line in baseline_output.splitlines() if "Disk" in line and (m := line.split(":")[-2:]) }If a line contains
"Disk"but doesn’t split cleanly into two":"parts (or the value isn’t numeric), this can raiseIndexErrororValueErrorand abort the test.A more defensive approach would be:
- baseline_output = self.session.cmd_output( - self.params["cmd_cat_io"] % baseline_io_file - ) - baseline_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in baseline_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + baseline_output = self.session.cmd_output( + self.params["cmd_cat_io"] % baseline_io_file + ) + baseline_metrics = {} + for line in baseline_output.splitlines(): + if "Disk" not in line: + continue + parts = line.split(":")[-2:] + if len(parts) < 2: + LOG_JOB.warning( + "Skipping unexpected IO metrics line in baseline: %r", line + ) + continue + try: + baseline_metrics[parts[0]] = float( + parts[1].split()[0].replace(",", "") + ) + except ValueError: + LOG_JOB.warning( + "Skipping non-numeric IO metrics value in baseline: %r", line + ) @@ - new_output = self.session.cmd_output(self.params["cmd_cat_io"] % new_io_file) - new_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in new_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + new_output = self.session.cmd_output(self.params["cmd_cat_io"] % new_io_file) + new_metrics = {} + for line in new_output.splitlines(): + if "Disk" not in line: + continue + parts = line.split(":")[-2:] + if len(parts) < 2: + LOG_JOB.warning( + "Skipping unexpected IO metrics line in new run: %r", line + ) + continue + try: + new_metrics[parts[0]] = float( + parts[1].split()[0].replace(",", "") + ) + except ValueError: + LOG_JOB.warning( + "Skipping non-numeric IO metrics value in new run: %r", line + )This keeps the intent (compare “Disk …” metrics before/after load) but won’t blow up if the script’s output format changes slightly.
Also applies to: 692-698
🧹 Nitpick comments (2)
qemu/tests/win_guest_debugging_tool.py (2)
30-47: Consider checking session liveness before reusingself.session.Right now
_get_session()will happily return an existingself.sessionwithout verifying it’s still alive. If the underlying channel has died (e.g. long‑running test, network hiccup), callers will keep using a dead session and see cascading command failures. Consider mirroring the pattern used elsewhere in the tree (e.g.provider/storage_benchmark.py:session()), by callingself.session.is_alive()and recreating the session when it’s not, before returning it.This would better align with the PR goal of “more robust” session management, while still keeping
_open_sessionstracking logic as is.
198-211: Use a file-appropriate delete forlog_zipcleanup to avoid silent leftovers.In
_check_run_tools_multi_times(),cmd_remove_dir(backed byrmdir /S /Q %sfrom the cfg) is used for both:
paths["log_folder"](a directory)paths["log_zip"](a file)On Windows,
rmdiragainst a regular file is unreliable and will typically fail, leaving the ZIP artifacts behind without being noticed (since exit status is ignored). Over multiple iterations/runs this can clutter the guest.Consider either:
- Introducing a separate
cmd_remove_file(usingdelorRemove-Item), and using it forlog_zip; or- Updating
cmd_remove_dirin the cfg to be a more generic “remove path if file or dir” wrapper (e.g. a small PowerShell one‑liner that checksTest-PathandGet-Itemtype).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(1 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (3)
provider/storage_benchmark.py (1)
session(83-90)provider/virt_storage/storage_volume.py (1)
info(202-217)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (3)
qemu/tests/win_guest_debugging_tool.py (2)
131-147: Variant dispatcher is clear and fail-fast.
_execute_test_variant()now cleanly mapswindegtool_check_typeto_check_<type>methods, with an explicit existence check and clear error message for unknown types. This makes it easy to add new variants and surfaces misconfigurations early.
705-723: Newexecute()+run()flow cleanly encapsulates the test lifecycle.Wrapping
setup()+ variant execution inexecute()with afinally: _cleanup_sessions()block, and havingrun()just instantiateWinDebugToolTestand callexecute(), is a much clearer and safer structure than the previous multi‑hook base class. It guarantees session cleanup even on failures and keeps the entry point straightforward.qemu/tests/cfg/win_guest_debugging_tool.cfg (1)
29-33: Stop-Process-ErrorAction SilentlyContinuealigns with “best-effort” interruption semantics.Extending
cmd_kill_powershell_process1to include-ErrorAction SilentlyContinueis a good fit here: it avoids treating “msinfo32.exe isn’t running” as a hard failure, while_check_user_friendliness()already logs status/output for diagnostics.
|
@leidwang could you please help to review when you have a moment? |
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
df6c911 to
ee7e8a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
qemu/tests/win_guest_debugging_tool.py (3)
223-226: Quote the script path in the PowerShell command (duplicate of earlier comment).This is the same path quoting issue as in
_run_script_and_get_paths. The script path should be quoted to handle spaces.Apply this diff:
cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" + f'-File "{self.script_path}" {param}' )Note: Line 241 has the same issue—apply the same fix there.
Based on learnings, this was flagged in the previous review.
85-88: Quote the script path in the PowerShell command.Per the past review comment, Windows paths should be quoted to handle spaces reliably. The script path in the
-Fileparameter is not quoted here, which will cause the command to fail if the path contains spaces (e.g., if the script is underC:\Program Files).Apply this diff:
cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {extra_args}" + f'-File "{self.script_path}" {extra_args}' )Note: Similar issues exist at lines 223-226 and 241 in
_check_user_friendliness. Apply the same fix there.Based on learnings, this was flagged in a previous review but not yet addressed.
76-76: Quote the path in thecdcommand.This command is vulnerable to paths containing spaces, which are common on Windows (e.g., under
C:\Program Files). While some commands in the code correctly quote paths (e.g., lines 121, 302, 484, 628), this one doesn't, creating inconsistency.Apply this diff:
- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(1 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/win_guest_debugging_tool.cfg
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
2 times, most recently
from
55a657b to
a6c3c50
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
qemu/tests/win_guest_debugging_tool.py (4)
704-708: Add length check to prevent IndexError.Same issue as lines 665-669: the dictionary comprehension needs a length check to ensure
mhas at least 2 elements before accessingm[1].new_metrics = { m[0]: float(m[1].split()[0].replace(",", "")) for line in new_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 }
665-669: Add length check to prevent IndexError.The walrus operator assigns the result of
split(":")[-2:]tom, but if a line has fewer than 2 colons,mwill have fewer than 2 elements, causingm[1]to raise anIndexError. This was flagged in a previous review but remains unaddressed in the current code.baseline_metrics = { m[0]: float(m[1].split()[0].replace(",", "")) for line in baseline_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 }Based on learnings, this issue was flagged previously but not yet fixed.
251-251: Quote the script path to handle spaces.Consistent with other locations, this path should be quoted.
- cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = f'powershell.exe -ExecutionPolicy Bypass -File "{self.script_path}"'
233-236: Quote the script path to handle spaces.The script path should be quoted to handle paths with spaces.
cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" + f'-File "{self.script_path}" {param}' )
🧹 Nitpick comments (4)
qemu/tests/win_guest_debugging_tool.py (4)
76-76: Consider quoting the path for robustness.While
tmp_dirtypically comes from configuration and is unlikely to contain spaces, quoting the path would make the code more defensive.- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
380-380: Consider quoting the path for consistency.Similar to line 76, quoting would improve robustness.
- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
432-432: Consider quoting the path for consistency.Same pattern as previous cd commands.
- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
631-631: Consider quoting the path for consistency.Same pattern as other cd commands.
- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(2 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🔇 Additional comments (12)
qemu/tests/win_guest_debugging_tool.py (9)
12-29: LGTM! Clean class initialization.The new self-contained class structure with proper instance variable initialization and session tracking is well-designed.
30-47: LGTM! Robust session management.The session tracking and reuse logic properly ensures all sessions are tracked for cleanup, addressing previous review concerns.
49-57: LGTM! Proper cleanup with defensive error handling.The exception handling ensures cleanup continues even if individual sessions fail to close.
127-139: LGTM! Proper path quoting and clear error reporting.The quoted path and missing file detection logic are well-implemented.
141-157: LGTM! Clean dispatcher with proper existence check.The
hasattrcheck beforegetattrproperly addresses the previous pylint concern.
159-168: LGTM! Clean and focused test method.Good delegation to the centralized helper method.
170-205: LGTM! Robust size comparison with proper error handling.The numeric size comparison properly addresses the previous string comparison concern.
207-220: LGTM! Straightforward stability test with proper cleanup.The loop with cleanup between iterations is appropriate for stability testing.
714-733: LGTM! Clean entry point with proper cleanup.The try/finally block ensures session cleanup occurs even if tests fail, and the new self-contained class structure is well-designed.
qemu/tests/cfg/win_guest_debugging_tool.cfg (3)
16-16: LGTM! Sensible default timeout.The 720-second (12-minute) default timeout is reasonable for Windows debugging script execution.
21-24: LGTM! Win2016-specific configuration is appropriate.The extended timeout and IE Enhanced Security Configuration commands properly support the test requirements for Windows Server 2016 guests.
37-37: LGTM! Defensive error handling for process termination.Adding
-ErrorAction SilentlyContinueprevents failures when the msinfo32 process doesn't exist, which is appropriate for interrupt testing scenarios.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
a6c3c50 to
cfd6e6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
qemu/tests/win_guest_debugging_tool.py (2)
93-102: Blocker: quote-Filepaths (still unquoted in multiple places). This will fail when the script path (or copied script + args) contains spaces and can also mis-parse arguments.- cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {extra_args}" - ) + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}" {extra_args}' + ) @@ - cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" - ) + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}" {param}' + ) @@ - cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}"' + ) @@ - full_command = ( - f"powershell.exe -ExecutionPolicy Bypass -File {ps_script_and_args}" - ) + # Quote the script token; keep args outside quotes + ps_script_and_args = ps_script_and_args.strip() + script_token, *rest = ps_script_and_args.split(" ", 1) + rest = rest[0] if rest else "" + full_command = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{script_token}" {rest}' + ).strip()Also applies to: 233-236, 251-252, 555-557
705-709: PreventIndexErrorin IO metrics parsing (still present). The comprehension assumes at least 2:-separated fields, but doesn’t enforce it.- baseline_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in baseline_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + baseline_metrics = { + m[0]: float(m[1].split()[0].replace(",", "")) + for line in baseline_output.splitlines() + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 + } @@ - new_metrics = { - m[0]: float(m[1].split()[0].replace(",", "")) - for line in new_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) - } + new_metrics = { + m[0]: float(m[1].split()[0].replace(",", "")) + for line in new_output.splitlines() + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 + }Also applies to: 744-748
🧹 Nitpick comments (4)
qemu/tests/cfg/win_guest_debugging_tool.cfg (1)
75-75: Makecmd_disable_devicenon-fatal when cmdlet/device isn’t available.Disable-PnpDevice/Get-PnpDevicecan fail (missing module, access denied, no match), which can turn an optional “best-effort” step into a hard test failure. Consider adding-ErrorAction SilentlyContinue(ortry/catch) and/orImport-Module PnpDevice -ErrorAction SilentlyContinue.- cmd_disable_device = powershell.exe -Command "Get-PnpDevice | Where-Object {$_.FriendlyName -like '*%s*' -or $_.InstanceId -like '*%s*'} | Disable-PnpDevice -Confirm:$false" + cmd_disable_device = powershell.exe -Command "Import-Module PnpDevice -ErrorAction SilentlyContinue; Get-PnpDevice -ErrorAction SilentlyContinue | Where-Object {$_.FriendlyName -like '*%s*' -or $_.InstanceId -like '*%s*'} | Disable-PnpDevice -Confirm:$false -ErrorAction SilentlyContinue"qemu/tests/win_guest_debugging_tool.py (3)
67-77: Quote/escapetmp_dirusages (mkdir/cd) to survive spaces. Right nowcd {self.tmp_dir}will break if the temp path contains spaces (common on Windows).- self.session.cmd_output(self.params["cmd_create_dir"] % self.tmp_dir) - self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd_output(self.params["cmd_create_dir"] % f'"{self.tmp_dir}"') + self.session.cmd(f'cd /d "{self.tmp_dir}"')
100-102: Include command output in the failure to speed up debugging.- if status != 0: - self.test.fail(f"Script execution failed with status {status}") + if status != 0: + self.test.fail( + f"Script execution failed with status {status}. Output:\n{output}" + )
438-444: Handle multi-adapter output fromGet-NetAdapter..strip()can leave multiple lines; the later commands expect a single adapter name.- adapter_name = self.session.cmd_output( - self.params["check_adapter_name"] - ).strip() + adapter_names = [ + l.strip() + for l in self.session.cmd_output(self.params["check_adapter_name"]).splitlines() + if l.strip() + ] + if not adapter_names: + self.test.error("No network adapters returned by Get-NetAdapter.") + adapter_name = adapter_names[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(3 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (2)
provider/storage_benchmark.py (1)
session(83-90)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (3)
qemu/tests/cfg/win_guest_debugging_tool.cfg (2)
16-24: Good: centralizedscript_execution_timeout, and Win2016-specific override/IE ESC toggles are clean.
37-37: Good:Stop-Process ... -ErrorAction SilentlyContinueavoids spurious failures whenmsinfo32isn’t running.qemu/tests/win_guest_debugging_tool.py (1)
30-57: Session tracking/cleanup looks solid (reused sessions are now tracked).
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
cfd6e6f to
a8e0c9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
qemu/tests/win_guest_debugging_tool.py (4)
93-96: Quote-Filepaths (still unquoted in multiple places). This will break when the script/workdir path contains spaces.- cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {extra_args}" - ) + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}" {extra_args}' + )- cmd = ( - "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" - ) + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}" {param}' + )- cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}"' + )- full_command = ( - f"powershell.exe -ExecutionPolicy Bypass -File {ps_script_and_args}" - ) + full_command = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{ps_script_and_args}"' + )Also applies to: 233-236, 251-252, 554-557
131-139: Fix file presence check to avoid substring false-pass/false-fail.- output = str(self.session.cmd_output(f'dir /b "{path}"')).strip() + output = str(self.session.cmd_output(f'dir /b "{path}"')).strip() + present = {line.strip() for line in output.splitlines() if line.strip()} target_files_key = "target_dump_files" if is_dump else "target_files" - target_files = self.params[target_files_key].split(",") + target_files = [t.strip() for t in self.params[target_files_key].split(",") if t.strip()] - missing_files = [f for f in target_files if f not in output] + missing_files = [f for f in target_files if f not in present]
215-221: Bug: removing{paths["log_zip"]}viarmdiris wrong (zip is a file). Can leave artifacts and break subsequent iterations.for i in range(5): @@ # Clean up for the next iteration self.session.cmd(self.params["cmd_remove_dir"] % paths["log_folder"]) - self.session.cmd(self.params["cmd_remove_dir"] % paths["log_zip"]) + status, _ = self.session.cmd_status_output(f'del /F /Q "{paths["log_zip"]}"') + if status != 0: + LOG_JOB.warning("Failed to delete zip: %s", paths["log_zip"])
708-712: Prevent possibleIndexErrorin IO metrics parsing.baseline_metrics = { m[0]: float(m[1].split()[0].replace(",", "")) for line in baseline_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 } @@ new_metrics = { m[0]: float(m[1].split()[0].replace(",", "")) for line in new_output.splitlines() - if "Disk" in line and (m := line.split(":")[-2:]) + if "Disk" in line and len(m := line.split(":")[-2:]) >= 2 }Also applies to: 747-751
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(3 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (3)
provider/storage_benchmark.py (1)
session(83-90)provider/virt_storage/storage_volume.py (1)
info(202-217)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg (2)
16-16: Timeout override looks good; consider scoping IE ESC commands to Win2016-only usage in code/docs.Also applies to: 21-24
37-37: Good: makeStop-Processnon-fatal (avoids breaking the test whenmsinfo32isn’t present).
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
3 times, most recently
from
7d52bc7 to
7c5ce8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
qemu/tests/win_guest_debugging_tool.py (7)
67-73: Handle multi-line results fromcmd_search_file_global(script path discovery is brittle).
self.session.cmd_output(...).strip()can preserve embedded newlines when multiple matches are returned, which then breaks later path usage.error_context.context("Locating debug tool script.", LOG_JOB.info) cmd = self.params["cmd_search_file_global"] % self.script_name - self.script_path = str(self.session.cmd_output(cmd, timeout=360)).strip() - if not self.script_path: + raw = str(self.session.cmd_output(cmd, timeout=360)) + matches = [l.strip() for l in raw.splitlines() if l.strip()] + if not matches: self.test.error(f"'{self.script_name}' not found on the guest.") + if len(matches) > 1: + LOG_JOB.warning( + "Multiple '%s' found; using first match: %r", + self.script_name, + matches[:5], + ) + self.script_path = matches[0] self.script_dir = self.script_path.replace(self.script_name, "").rstrip("\\")
93-125: Include script output in non-zero exit failures (improves triage).Right now you drop the output, which is usually the only actionable error.
status, output = self.session.cmd_status_output(cmd, timeout=script_timeout) if status != 0: - self.test.fail(f"Script execution failed with status {status}") + self.test.fail( + f"Script execution failed with status {status}. Output:\n{output}" + )
182-187: Add timeout (and surface output) for ZIP extraction to avoid hangs.cmd_extract = self.params["cmd_extract_zip"] % (log_zip, extract_folder) - status, _ = self.session.cmd_status_output(cmd_extract) + script_timeout = int(self.params.get("script_execution_timeout", 720)) + status, extract_out = self.session.cmd_status_output( + cmd_extract, timeout=script_timeout + ) if status != 0: - self.test.error("Failed to extract the generated ZIP file.") + self.test.error( + f"Failed to extract the generated ZIP file. Output:\n{extract_out}" + )
216-222: Bug: deleting{paths["log_zip"]}via remove-dir command is wrong (it’s a file).This can fail and/or leave artifacts across iterations.
for i in range(5): @@ # Clean up for the next iteration self.session.cmd(self.params["cmd_remove_dir"] % paths["log_folder"]) - self.session.cmd(self.params["cmd_remove_dir"] % paths["log_zip"]) + # Zip is a file, not a directory + self.session.cmd(f'del /F /Q "{paths["log_zip"]}"')
234-238: Quote-Filescript paths in_check_user_friendliness(breaks on spaces).You fixed quoting in
_run_script_and_get_paths(), but this variant regressed.cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" + f'-File "{self.script_path}" {param}' ) @@ - cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = ( + "powershell.exe -ExecutionPolicy Bypass " + f'-File "{self.script_path}"' + )Also applies to: 252-253
439-445: Make adapter selection + Jumbo Packet parsing robust (multi-NIC + non-integer output).
Get-NetAdapter | Select -Expand Namecan return multiple lines, andDisplayValueis often not a pure integer;int(cmd_output(...))will throw or mis-parse.+ def _parse_int(s: str) -> int: + m = re.search(r"\d+", s.replace(",", "")) + if not m: + raise ValueError(f"No integer token in output: {s!r}") + return int(m.group(0)) + - adapter_name = self.session.cmd_output( - self.params["check_adapter_name"] - ).strip() - original_jp_value = int( - self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) - ) + adapter_names = [ + l.strip() + for l in self.session.cmd_output(self.params["check_adapter_name"]).splitlines() + if l.strip() + ] + if not adapter_names: + self.test.error("No network adapters found on guest.") + adapter_name = adapter_names[0] + original_jp_value = _parse_int( + self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) + ) @@ - current_jp_value = int( - self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) - ) + current_jp_value = _parse_int( + self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) + ) @@ - final_jp_value = int( - self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) - ) + final_jp_value = _parse_int( + self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) + )Also applies to: 462-466, 508-513
131-137: Striptarget_filesentries before exact matching.If the cfg has spaces after commas,
f not in presentwill false-fail.- target_files = self.params[target_files_key].split(",") + target_files = [f.strip() for f in self.params[target_files_key].split(",") if f.strip()]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(4 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/win_guest_debugging_tool.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (2)
provider/storage_benchmark.py (1)
session(83-90)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
7c5ce8f to
3f0289a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
qemu/tests/win_guest_debugging_tool.py (7)
74-76: Quote thecdpath to handle spaces.Windows paths commonly contain spaces (e.g.,
C:\Program Files). The unquotedcdcommand will fail on such paths.Apply this diff:
error_context.context("Creating temporary work directory.", LOG_JOB.info) self.session.cmd_output(self.params["cmd_create_dir"] % self.tmp_dir) - self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')Note: Apply the same fix at lines 408, 460, 507, 675, and 688 where
cdis also used without quotes.
183-186: Add timeout to ZIP extraction to prevent hangs.Large ZIP files can take significant time to extract. Without a timeout, the test can hang indefinitely if extraction stalls.
Apply this diff:
extract_folder = f"{log_folder}_extracted" cmd_extract = self.params["cmd_extract_zip"] % (log_zip, extract_folder) - status, _ = self.session.cmd_status_output(cmd_extract) + script_timeout = int(self.params.get("script_execution_timeout", 720)) + status, _ = self.session.cmd_status_output(cmd_extract, timeout=script_timeout) if status != 0: self.test.error("Failed to extract the generated ZIP file.")
216-221: Bug: Using directory removal command on ZIP file.Line 221 uses
cmd_remove_dir(which isrmdir /S /Q) onpaths["log_zip"], but ZIP files are files, not directories. This will fail on Windows.Apply this diff:
for i in range(5): LOG_JOB.info("Running iteration %d...", i + 1) paths = self._run_script_and_get_paths() # Clean up for the next iteration self.session.cmd(self.params["cmd_remove_dir"] % paths["log_folder"]) - self.session.cmd(self.params["cmd_remove_dir"] % paths["log_zip"]) + # ZIP is a file, not a directory + self.session.cmd(f'del /F /Q "{paths["log_zip"]}"')
234-243: Quote script path to handle spaces.The PowerShell command at line 236 does not quote
self.script_path, which will fail if the path contains spaces (common on Windows).Apply this diff:
for param in invalid_params: cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" + f'-File "{self.script_path}" {param}' ) output = self.session.cmd_output(cmd, timeout=360)
439-444: Handle multi-line adapter output and non-integer Jumbo Packet values.If the guest has multiple network adapters,
check_adapter_namewill return multiple lines, but line 440 only uses.strip(), which keeps newlines. Line 443 also assumes the Jumbo Packet display value is a pure integer, but it may contain units or formatting.Apply this diff:
- adapter_name = self.session.cmd_output( - self.params["check_adapter_name"] - ).strip() + adapter_names = [ + l.strip() + for l in self.session.cmd_output(self.params["check_adapter_name"]).splitlines() + if l.strip() + ] + if not adapter_names: + self.test.error("No network adapters found on guest.") + adapter_name = adapter_names[0] + + def _parse_jp_int(s: str) -> int: + """Extract integer from potentially formatted output.""" + m = re.search(r"\d+", s.replace(",", "")) + if not m: + raise ValueError(f"No integer found in {s!r}") + return int(m.group(0)) + - original_jp_value = int( - self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) - ) + original_jp_value = _parse_jp_int( + self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) + )Note: Apply the same
_parse_jp_inthelper at lines 462-464 and 508-510 for consistency.
67-72: Handle multi-line results from script search (can breakscript_path).If
cmd_search_file_globalreturns multiple matching files (e.g., script exists in multiple locations),.strip()keeps embedded newlines. This will cause subsequent path operations (like constructingscript_dir) to fail or behave incorrectly.Apply this diff to safely extract the first match:
error_context.context("Locating debug tool script.", LOG_JOB.info) cmd = self.params["cmd_search_file_global"] % self.script_name - self.script_path = str(self.session.cmd_output(cmd, timeout=360)).strip() - if not self.script_path: + raw = str(self.session.cmd_output(cmd, timeout=360)) + matches = [l.strip() for l in raw.splitlines() if l.strip()] + if not matches: self.test.error(f"'{self.script_name}' not found on the guest.") + if len(matches) > 1: + LOG_JOB.warning("Multiple '%s' found; using first: %r", self.script_name, matches[:5]) + self.script_path = matches[0] self.script_dir = self.script_path.replace(self.script_name, "").rstrip("\\")
551-551: Config commandcmd_cp_fileneeds quotes around path placeholders.The current config defines
cmd_cp_fileaspowershell.exe -Command "cp %s %s -Force". When%sis replaced with a path containing spaces, the PowerShellcpcmdlet will fail because the path arguments are not quoted within the-Commandstring.Update the config (in
qemu/tests/cfg/win_guest_debugging_tool.cfgline 105) to:- cmd_cp_file = powershell.exe -Command "cp %s %s -Force" + cmd_cp_file = powershell.exe -Command "Copy-Item '%s' '%s' -Force"Or update this test code to explicitly quote paths when using the command:
- self.session.cmd(self.params["cmd_cp_file"] % (self.script_path, ".")) + self.session.cmd(f'powershell.exe -Command "Copy-Item \\"{self.script_path}\\" . -Force"')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(4 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (2)
provider/storage_benchmark.py (1)
session(83-90)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (8)
qemu/tests/cfg/win_guest_debugging_tool.cfg (1)
16-16: LGTM! Configuration improvements enhance robustness.The configuration changes add sensible defaults and defensive patterns:
- Default script execution timeout with Win2016 override for longer operations
- IE Enhanced Security Configuration disable commands for test environment preparation
- Error handling flags (
-ErrorAction SilentlyContinue) prevent spurious failures- Device disable command uses defensive checks before attempting operations
These align well with the test refactor and improve reliability across Windows versions.
Also applies to: 21-24, 37-37, 75-75, 105-105
qemu/tests/win_guest_debugging_tool.py (7)
12-29: LGTM! Clean class design with proper session tracking.The consolidated
WinDebugToolTestclass eliminates the previous multi-stage inheritance complexity. The_open_sessionslist ensures proper cleanup of all VM sessions.
30-48: LGTM! Session management properly tracks all instances.The method correctly ensures that both reused and newly created sessions are added to
_open_sessions, preventing session leaks during cleanup.
86-125: LGTM! Script execution properly validates status and parses output.The method correctly:
- Quotes the script path to handle spaces
- Checks the exit status before parsing output
- Extracts log and dump folder paths via regex
- Validates generated files in each folder
127-140: LGTM! File presence check uses exact matching.The method correctly creates a set of actual filenames from
dir /boutput and checks for exact matches, avoiding false positives from substring matching.
142-158: LGTM! Dynamic dispatch correctly validates method existence.The
hasattrcheck beforegetattrensures the method exists, avoiding both runtime errors and the pylint E1102 false positive.
709-713: LGTM! IO metrics parsing safely handles malformed output.The dictionary comprehensions correctly check
len(m) >= 2before accessingm[0]andm[1], preventingIndexErroron lines with unexpected formatting.Also applies to: 748-752
758-777: LGTM! Clean execution flow with proper cleanup.The
executemethod ensures_cleanup_sessionsruns even if the test fails, and therunfunction provides a clean entry point that replaces the previous multi-class lifecycle.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
3f0289a to
ffb9dba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
qemu/tests/win_guest_debugging_tool.py (9)
408-409: Quote thecdpath after reboot (same issue as line 76).- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
249-259: Quote the script path in interruption test (unresolved from previous review).Previous review at lines 249-259 flagged this same quoting issue.
try: # This is expected to time out when the process is terminated - cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = f'powershell.exe -ExecutionPolicy Bypass -File "{self.script_path}"'
74-77: Quote thecdpath to handle spaces (regression from previous review).Previous review at lines 74-77 flagged that unquoted
cdcommands will break ifself.tmp_dircontains spaces, which is common on Windows (e.g.,C:\Program Files).- self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')Note: This same issue appears at lines 408, 460, 693, and should be fixed consistently throughout.
183-184: Add timeout to ZIP extraction to avoid hangs (unresolved from previous review).Previous review at lines 183-187 requested adding a timeout parameter, but it's still missing.
- status, _ = self.session.cmd_status_output(cmd_extract) + script_timeout = int(self.params.get("script_execution_timeout", 720)) + status, _ = self.session.cmd_status_output(cmd_extract, timeout=script_timeout)
687-694: Quote thecdpath after BSOD reboot (same issue).self.session = self._get_session(new=True) - self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
459-460: Quote thecdpath in error recovery (same issue).# Re-establish session if it was dropped self.session = self._get_session(new=True) - self.session.cmd(f"cd {self.tmp_dir}") + self.session.cmd(f'cd "{self.tmp_dir}"')
216-221: Critical bug: Using directory removal command on a file (unresolved from previous review).Previous review at lines 216-222 flagged that
paths["log_zip"]is a file, not a directory, socmd_remove_dirwill fail. This can leave artifacts and break subsequent iterations.# Clean up for the next iteration self.session.cmd(self.params["cmd_remove_dir"] % paths["log_folder"]) - self.session.cmd(self.params["cmd_remove_dir"] % paths["log_zip"]) + # Zip is a file, not a directory + self.session.cmd(f'del /F /Q "{paths["log_zip"]}"')
67-73: Handle multi-line results from script search (regression from previous review).Previous review flagged that
cmd_search_file_globalcan return multiple matches with embedded newlines, breaking path operations. The current code still uses.strip()without splitting lines, so this issue persists.Apply this fix to handle multi-line output:
- self.script_path = str(self.session.cmd_output(cmd, timeout=360)).strip() - if not self.script_path: + raw = str(self.session.cmd_output(cmd, timeout=360)) + matches = [l.strip() for l in raw.splitlines() if l.strip()] + if not matches: self.test.error(f"'{self.script_name}' not found on the guest.") + if len(matches) > 1: + LOG_JOB.warning("Multiple '%s' found; using first: %r", self.script_name, matches[:5]) + self.script_path = matches[0]
234-243: Quote the script path to handle spaces (appears in multiple locations).Similar to other locations, the script path needs quoting here.
cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {param}" + f'-File "{self.script_path}" {param}' )
🧹 Nitpick comments (1)
qemu/tests/win_guest_debugging_tool.py (1)
100-102: Consider including command output in failure message for faster triage.While the status check is good, including the script output in the failure message would help diagnose issues more quickly.
if status != 0: - self.test.fail(f"Script execution failed with status {status}") + self.test.fail( + f"Script execution failed with status {status}. Output:\n{output}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(4 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/win_guest_debugging_tool.py (2)
provider/storage_benchmark.py (1)
session(83-90)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (14)
qemu/tests/cfg/win_guest_debugging_tool.cfg (1)
16-16: LGTM! Configuration changes properly address previous reviews.The added timeout configurations, Win2016-specific settings, and improved error handling for PowerShell commands align well with the test refactoring. The
-ErrorAction SilentlyContinueadditions and defensive device disable logic make the commands more resilient.Also applies to: 21-24, 37-37, 75-75, 105-105
qemu/tests/win_guest_debugging_tool.py (13)
127-140: LGTM! File presence check properly addresses previous review.The use of
splitlines()and set comparison with exact filename matching correctly addresses the previous substring matching issue.
142-158: LGTM! Test dispatcher properly addresses previous pylint issue.The use of
hasattrbeforegetattrcleanly resolves the E1102 error flagged in previous reviews.
160-169: LGTM! Script execution check is straightforward and correct.
188-206: LGTM! Folder size comparison properly addresses previous review.The numeric parsing with error handling correctly addresses the previous string comparison brittleness issue.
286-326: LGTM! Interruption signal file verification logic is robust.The fallback mechanism and path construction logic handle various edge cases well.
340-358: LGTM! System name extraction properly handles multi-line output.The loop-based parsing with line splitting is a good approach for extracting the system name.
469-490: LGTM! Network file verification logic is well-structured.The multi-space splitting and column extraction approach is appropriate for the expected output format.
514-571: LGTM! Documentation verification is comprehensive.The logic properly checks for standard docs and tests command examples from README.
573-680: LGTM! Registry collection test is thorough and well-structured.The helper functions, error handling, and cleanup in the finally block are all well-implemented.
695-709: LGTM! Sensitive data collection verification is appropriate.The BSOD trigger, dump file verification, and script execution with
-IncludeSensitiveDataare properly implemented.
724-731: LGTM! IO metrics parsing properly addresses previous review.The defensive
len(m := line.split(":")[-2:]) >= 2checks correctly prevent IndexError on malformed lines.Also applies to: 764-770
776-795: LGTM! Test execution flow and entry point are clean.The try-finally structure ensures proper session cleanup, and the entry point properly instantiates and executes the test.
93-96: Quote the script path to handle spaces (regression from previous review).Previous review at lines 93-96 was marked as "Addressed in commit 7c5ce8f", but the script path is still not quoted. Paths with spaces (common on Windows) will cause command failures.
cmd = ( "powershell.exe -ExecutionPolicy Bypass " - f"-File {self.script_path} {extra_args}" + f'-File "{self.script_path}" {extra_args}' )Likely an incorrect or invalid review comment.
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
ffb9dba to
15566eb
Compare
| Win2016: | ||
| script_execution_timeout = 1200 | ||
| cmd_disable_ie_esc_admin = powershell.exe -Command "Set-ItemProperty -Path 'HKLM:\SOFTWARE\Microsoft\Active Setup\Installed Components\{A509B1A7-37EF-4b3f-8CFC-4F3A74704073}' -Name 'IsInstalled' -Value 0" | ||
| cmd_disable_ie_esc_user = powershell.exe -Command "Set-ItemProperty -Path 'HKLM:\SOFTWARE\Microsoft\Active Setup\Installed Components\{A509B1A8-37EF-4b3f-8CFC-4F3A74704073}' -Name 'IsInstalled' -Value 0" |
There was a problem hiding this comment.
As we dicsussed offline, it would be better to break down current commit, it would be more clear.
There was a problem hiding this comment.
Yeah, I'll split another one, as for this one, just keep it here this time. thanks
The test script for the Windows guest debugging tool was
significantly refactored to improve its structure,
eliminate redundancy, and increase maintainability.
Key logical improvements include:
- The complex three-class inheritance structure was
consolidated into a single, self-contained WinDebugToolTest
class, simplifying the overall architecture.
- A centralized helper method, _run_script_and_get_paths,
was created to handle all PowerShell script executions and
output parsing. This removes significant code duplication
that was present in each test variant.
- Individual test methods were streamlined to focus purely on
their specific validation logic, delegating common tasks like
script execution and session management to helper functions.
- VM session management was made more robust to ensure sessions
are properly created, reused, and cleaned up.
Signed-off-by: Dehan Meng <demeng@redhat.com>
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
15566eb to
22c5f9c
Compare
New requirement from customer that collect MTV Firstboot log.txt by our script 'CollectSystemInfo.ps1'. Test steps are: 1. Check the 'C:\Program Files\Guestfs\Firstboot\log.txt' existed or not, if not, will create a corresponding exact file. 2. Run script 3. Check the file existed or not under generated directory. Signed-off-by: Dehan Meng <demeng@redhat.com>
6-dehan
force-pushed
the
KVMAUTOMA-4819-cleanup-code
branch
from
22c5f9c to
b4e4f2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
qemu/tests/win_guest_debugging_tool.py (3)
224-229: Critical: Using wrong command to delete ZIP file.Line 229 uses
cmd_remove_dirto deletepaths["log_zip"], but ZIP files are files, not directories. This will fail and leave artifacts, potentially causing subsequent test iterations to fail.Apply this diff:
# Clean up for the next iteration self.session.cmd(self.params["cmd_remove_dir"] % paths["log_folder"]) - self.session.cmd(self.params["cmd_remove_dir"] % paths["log_zip"]) + # Use file deletion for the zip file + self.session.cmd(f'del /F /Q "{paths["log_zip"]}"')Based on learnings, this was flagged in a previous review but remains unaddressed.
257-268: Quote script path to handle spaces.Line 260 does not quote
self.script_path, which will cause command failures if the path contains spaces.Apply this diff:
# This is expected to time out when the process is terminated - cmd = f"powershell.exe -ExecutionPolicy Bypass -File {self.script_path}" + cmd = f'powershell.exe -ExecutionPolicy Bypass -File "{self.script_path}"'Based on learnings, this was flagged in a previous review and supposedly addressed in commit ffb9dba, but the issue persists.
447-452: Make adapter selection and Jumbo Packet parsing robust.Line 449 uses
.strip()on the adapter name output, which will fail if multiple network adapters are present (multi-NIC systems return multiple lines). Line 451 usesint()directly on the Jumbo Packet display value, which often includes units like "9014 Bytes" and will raiseValueError.Apply this diff:
- adapter_name = self.session.cmd_output( - self.params["check_adapter_name"] - ).strip() - original_jp_value = int( - self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) - ) + adapter_names = [ + l.strip() + for l in self.session.cmd_output(self.params["check_adapter_name"]).splitlines() + if l.strip() + ] + if not adapter_names: + self.test.error("No network adapters found on guest.") + adapter_name = adapter_names[0] + + def _parse_jp_value(s: str) -> int: + """Extract integer from JP display value (e.g., '9014 Bytes' -> 9014).""" + m = re.search(r"\d+", s.replace(",", "")) + if not m: + raise ValueError(f"No integer in {s!r}") + return int(m.group(0)) + + original_jp_value = _parse_jp_value( + self.session.cmd_output(self.params["check_adapter_jp_info"] % adapter_name) + )Apply the same
_parse_jp_valuehelper at lines 470-471 and 516-517.Based on learnings, this was flagged in previous reviews and reportedly addressed in commit 2f7302a, but the issues remain.
🧹 Nitpick comments (1)
qemu/tests/win_guest_debugging_tool.py (1)
108-109: Include script output in the failure message.The failure message only reports the exit status. Including the command output would significantly speed up debugging of script execution failures.
Apply this diff:
if status != 0: - self.test.fail(f"Script execution failed with status {status}") + self.test.fail( + f"Script execution failed with status {status}. Output:\n{output}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/win_guest_debugging_tool.cfg(4 hunks)qemu/tests/win_guest_debugging_tool.py(1 hunks)
🔇 Additional comments (9)
qemu/tests/cfg/win_guest_debugging_tool.cfg (3)
75-75: Previous vulnerability has been properly resolved.The
cmd_disable_devicecommand now includes proper error handling and conditional logic. The change from a bare pipeline to a guardedif ($d)check ensures the command succeeds when no matching device is found or cmdlets are unavailable—this addresses the issue flagged in prior review feedback.
21-24: IE Enhanced Security Configuration registry paths verified—no issues found.The
cmd_disable_ie_esc_adminandcmd_disable_ie_esc_usercommands use the correct and standard registry GUIDs for disabling Internet Explorer Enhanced Security Configuration on Windows Server 2016. The GUID A509B1A7-37EF-4b3f-8CFC-4F3A74704073 is for admins and A509B1A8-37EF-4b3f-8CFC-4F3A74704073 is for users, with no documented variations across Win2016 editions.
112-120: MTV_firstboot_log_collection variant is fully integrated.The config variant at lines 112-120 correctly aligns with the test harness implementation. The dispatcher method (lines 152-159) dynamically routes
windegtool_check_type = MTV_firstboot_log_collectionto the_check_MTV_firstboot_log_collectionmethod, and all config keys (firstboot_dir, firstboot_file, firstboot_content, collected_file_name, cmd_create_firstboot_dir, cmd_create_firstboot_file, cmd_check_file_content) are properly referenced in the implementation.qemu/tests/win_guest_debugging_tool.py (6)
12-28: LGTM! Clean class structure.The refactored single-class design with explicit session tracking via
_open_sessionsis a solid improvement over the previous multi-class inheritance pattern.
30-57: LGTM! Robust session management.The session tracking ensures all sessions are properly cleaned up, and the exception handling in
_cleanup_sessionsprevents cascading failures during teardown.
59-92: LGTM! Robust script discovery and setup.The multi-line handling for
cmd_search_file_globaloutput (lines 70-79) correctly addresses the potential for multiple matches, and path quoting is properly applied.
135-148: LGTM! Correct file presence validation.The set-based exact filename matching (lines 140, 144) properly avoids false positives from substring matches like
foo.txtmatchingmyfoo.txt.
735-738: LGTM! Safe dictionary comprehension.The defensive length checks (
len(m := line.split(":")[-2:]) >= 2) correctly preventIndexErrorwhen parsing disk metrics, ensuring the code handles unexpected output formats gracefully.Also applies to: 774-777
849-868: LGTM! Clean entry point design.The
execute()method with try/finally ensures_cleanup_sessions()always runs, and therun()function properly instantiates and invokes the test class. This is a clean replacement for the previous multi-class lifecycle pattern.
2 participants
The test script for the Windows guest debugging tool was
significantly refactored to improve its structure,
eliminate redundancy, and increase maintainability.
Key logical improvements include:
ID: 3274
Signed-off-by: Dehan Meng demeng@redhat.com
Summary by CodeRabbit
Tests
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.