Add virtio-vga visual test with AI-based verification #4432
Conversation
WalkthroughAdds a new Windows-focused virtio-vga visual test consisting of a config file Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @qemu/tests/virtio_vga_visual_test.py:
- Around line 423-425: The code assumes delete_file_cmd is present and does
self.params.get("delete_file_cmd") % guest_img_path which raises a TypeError if
the param is missing; update the logic in the block using delete_cmd (the
variable assigned from self.params.get("delete_file_cmd")) to validate existence
before formatting and executing: fetch the param into delete_cmd, check if
delete_cmd is truthy (or provide a sensible default/skip behavior), only perform
the % guest_img_path interpolation when delete_cmd is a string, and then call
self.session.cmd_status(delete_cmd, timeout=30) (or skip/log a warning when the
param is absent) so the code does not attempt string formatting on None.
- Around line 1225-1228: The reboot call inside run_reset_with_driver is missing
the session argument which breaks session tracking; update the vm.reboot(...)
call in the reset loop to pass the current session (use session=self.session as
done in run_reboot_with_driver) so the reboot reuses/tracks the correct session
and then continue appending self.session to self._open_session_list.
🧹 Nitpick comments (7)
qemu/tests/cfg/virtio_vga_visual_test.cfg (1)
46-46: Internal URL may not be accessible in all environments.The
gvncviewer_tool_urlpoints to an internal Red Hat server (download.eng.pek2.redhat.com). This could cause test failures in environments without access to this internal network.Consider making this URL configurable or providing a fallback mechanism.
qemu/tests/virtio_vga_visual_test.py (6)
61-77: Silent exception swallowing may hide configuration issues.The broad
except Exceptionblock silently ignores any errors when reading/etc/redhat-release. If the file exists but has unexpected content or permissions issues, the test will silently fall back to RHEL 9, which could lead to incorrect display server selection.Consider logging the exception at debug level to aid troubleshooting.
🔎 Proposed fix
except Exception: - pass + LOG_JOB.debug("Failed to detect RHEL version, defaulting to 9") # Default to RHEL 9 self.rhel_version = 9
110-138: Lock file presence doesn't guarantee Xvfb is running.Checking only for the lock file existence (line 117) doesn't verify the Xvfb process is actually running. The lock file could be stale from a crashed process. Consider verifying the process is alive before returning success.
🔎 Proposed fix
# Check if display already running by checking lock file lock_file = "/tmp/.X%s-lock" % self.display_num if os.path.exists(lock_file): + # Verify the process is actually running + try: + with open(lock_file, "r") as f: + pid = int(f.read().strip()) + os.kill(pid, 0) # Check if process exists + except (ValueError, OSError, ProcessLookupError): + # Stale lock file, remove it + LOG_JOB.warning("Removing stale Xvfb lock file: %s", lock_file) + os.remove(lock_file) + else: + os.environ["DISPLAY"] = display + return True - os.environ["DISPLAY"] = display - return True
297-309: VNC port calculation assumes standard base port.The calculation
vnc_display = vnc_port - 5900assumes VNC always uses the standard base port. While this is typically correct, if QEMU uses a non-standard VNC configuration, this could produce incorrect display numbers.
348-354: Hard-coded screenshot path reduces flexibility.The default screenshot path
/home/desktop.pngis hard-coded. If thegvncviewer-customizetool changes its output location or if multiple concurrent tests run, this could cause issues.
836-845: Usinggetattron VM object with potentially undefined attribute.
getattr(self.vm, "REBOOT_TIMEOUT", 240)retrieves a class/instance attribute, butREBOOT_TIMEOUTis likely meant to be a param, not a VM attribute. Consider usingself.params.get("reboot_timeout", 240)for consistency with other timeout handling in this file.🔎 Proposed fix
def _wait_for_shutdown(self): """Wait for VM to shutdown, then restart.""" error_context.context("Waiting for VM to shutdown", LOG_JOB.info) - timeout = getattr(self.vm, "REBOOT_TIMEOUT", 240) + timeout = int(self.params.get("reboot_timeout", 240)) if not utils_misc.wait_for(self.vm.is_dead, timeout=timeout): self.test.fail("VM failed to shutdown within %d seconds" % timeout)
1013-1028: Document the meaning of Windows exit status codes.Status codes
3010(ERROR_SUCCESS_REBOOT_REQUIRED) and259(STILL_ACTIVE) have specific meanings in Windows. Adding a brief comment would improve code clarity.🔎 Proposed fix
+ # 0: Success + # 3010: ERROR_SUCCESS_REBOOT_REQUIRED + # 259: STILL_ACTIVE (operation pending) if status in (0, 3010, 259): removed_count += 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_vga_visual_test.cfgqemu/tests/virtio_vga_visual_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_vga_visual_test.py (1)
provider/win_driver_utils.py (1)
install_driver_by_virtio_media(159-194)
🔇 Additional comments (8)
qemu/tests/cfg/virtio_vga_visual_test.cfg (2)
1-19: LGTM!The test metadata, driver configuration, and devcon path setup are well-structured with appropriate architecture-specific handling for i386/x86_64.
52-95: LGTM!The test variants are well-organized, covering a comprehensive set of scenarios including basic interaction, shutdown/reboot/reset cycles, driver lifecycle tests, and resolution modifications.
qemu/tests/virtio_vga_visual_test.py (6)
1-13: LGTM!Imports are appropriate for the module's functionality, and the logger is correctly configured using the avocado test logger.
194-221: RHEL 10+ setup path does not start the display server.For RHEL 10+,
install_xwayland()is called but there's no correspondingstart_xwayland()method or call. The_vnc_screendumpmethod (lines 314-327) wraps commands withxwfb-runfor RHEL 10+, but the display environment may not be properly initialized before tool installation.Verify this is intentional and that
xwfb-runhandles display initialization on-demand.
571-582: Key sending logic executes after command completion, not during.The
key_to_install_drivermechanism sends keys aftercmd_status_outputreturns with a non-zero status. However, if the command is waiting for user interaction, it would timeout or fail before reaching this code. This logic appears to be intended for handling driver installation prompts, but the sequencing seems incorrect.Verify this pattern matches the expected behavior for handling Windows driver installation dialogs.
1369-1388: Stub methods correctly fail with descriptive messages.The not-yet-implemented test actions properly call
self.test.fail()with clear messages indicating they're not implemented. This is the appropriate pattern for work-in-progress test scaffolding.
1528-1559: LGTM!The main
run()method has a clean structure with proper error handling via try/finally for cleanup, dynamic test dispatch using getattr, and appropriate VNC environment initialization gating.
1562-1566: LGTM!The entry point function correctly instantiates the test class and delegates to the
run()method.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @qemu/tests/virtio_vga_visual_test.py:
- Around line 99-108: Typo: the code checks for and invokes "xwfb-run" but
should use "xwayland-run". In install_xwayland(), replace the check that runs
which xwfb-run with which xwayland-run so the detected command matches the
package being installed, and likewise update the later invocation (the call that
runs xwfb-run around line 318) to invoke xwayland-run; update any related
_run_cmd calls and string literals that set or rely on the command name (e.g.,
in install_xwayland and the runtime invocation) so both checks and executions
consistently use "xwayland-run".
- Around line 677-687: Add an upfront Gemini API availability check (either in
_ensure_vnc_environment() or at the start of run()) so
verify_screen_with_gemini() calls (used around response handling and diagnostics
in this file) fail fast with a clear diagnostic; implement by invoking a
lightweight ppm_utils API-check helper (or add one if missing) that validates
credentials and endpoint reachability, and if it fails raise a clear
TestFail/RuntimeError with an explanatory message so subsequent calls to
ppm_utils.verify_screen_with_gemini() (used at the primary checks and
diagnostics) are not attempted when Gemini is misconfigured or unreachable.
♻️ Duplicate comments (2)
qemu/tests/virtio_vga_visual_test.py (2)
423-425: Potential TypeError ifdelete_file_cmdparameter is missing.This issue was previously identified. If
delete_file_cmdis not defined in params, the%formatting will raise aTypeError.🔎 Proposed fix
- delete_cmd = self.params.get("delete_file_cmd") % guest_img_path - self.session.cmd_status(delete_cmd, timeout=30) + delete_cmd_template = self.params.get("delete_file_cmd") + if delete_cmd_template: + delete_cmd = delete_cmd_template % guest_img_path + self.session.cmd_status(delete_cmd, timeout=30)
1225-1228: Inconsistent reboot call - missingsessionparameter.This issue was previously identified. The
vm.reboot()call doesn't pass thesessionparameter, unlike similar calls in other methods (e.g., line 1200-1201). This breaks session tracking.🔎 Proposed fix
for i in range(5): error_context.context("Reset loop %d/5" % (i + 1), LOG_JOB.info) - self.session = self.vm.reboot(method="system_reset", timeout=reboot_timeout) + self.session = self.vm.reboot( + session=self.session, method="system_reset", timeout=reboot_timeout + ) self._open_session_list.append(self.session)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_vga_visual_test.cfgqemu/tests/virtio_vga_visual_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_vga_visual_test.py (1)
provider/win_driver_utils.py (1)
install_driver_by_virtio_media(159-194)
🔇 Additional comments (5)
qemu/tests/cfg/virtio_vga_visual_test.cfg (1)
1-94: Configuration structure looks solid.The test configuration is well-organized with appropriate timeouts, Windows guest commands, AI verification prompts, and comprehensive test variants. The WIN_UTILS placeholder approach for dynamic path resolution is a good design choice.
qemu/tests/virtio_vga_visual_test.py (4)
38-59: Safe subprocess usage pattern.The
_run_cmdmethod properly handles both shell and non-shell command execution with appropriate timeout and error handling. The conditional shell usage is controlled and safe for this test framework context.
483-616: Well-structured driver update implementation.The
_update_drivermethod handles both install and upgrade scenarios appropriately, with proper INF discovery, version tracking, and verification. Good handling of devcon command statuses (0, 1) and driver verification via driverquery.
936-1051: Robust driver store cleanup with regex-based parsing.The
_clean_driver_storemethod uses sophisticated regex parsing to identify driver packages and handles both Windows 2016 and newer versions correctly. The dual approach (PnPUtil + WMIC fallback) increases reliability.
1396-1505: Comprehensive resolution verification with multi-layer checks.The manual resolution modification test properly validates resolution changes through three mechanisms: Windows API queries, VNC screenshot dimension verification (with reasonable 10px tolerance), and AI-powered visual quality checks. This multi-layer approach effectively catches different types of failures.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @qemu/tests/virtio_vga_visual_test.py:
- Around line 572-579: The current check formats chk_cmd with a truncated
device_name (device_name[0:30]) which can produce false positives; update the
formatting to use the full device_name when building chk_cmd (keep chk_cmd,
device_name, session.cmd_status, wait_for, and driver_name as the reference
points) and, if needed, make the match more specific (for example use an
exact/quoted match or a more strict driverquery/find pattern) so the wait_for
result reliably targets the intended driver rather than a prefix match.
🧹 Nitpick comments (7)
qemu/tests/cfg/virtio_vga_visual_test.cfg (1)
52-94: Test variants are comprehensive.The variants cover essential driver lifecycle scenarios. Note that several variants (
check_driver_signature_signtool,check_driver_signature_sigverif,boot_multiple_drivers,install_driver_iommu,desktop_migration, etc.) are defined here but will fail with "not yet implemented" errors per the Python implementation. Consider either:
- Adding a
TODOcomment noting which variants are stubs- Temporarily commenting out unimplemented variants to prevent accidental CI failures
qemu/tests/virtio_vga_visual_test.py (6)
65-81: Silent exception swallowing hides diagnostic information.The bare
except Exception: pass(lines 76-77) will silently hide any errors during RHEL version detection. Consider logging the exception for debugging purposes.Proposed fix
try: if os.path.exists("/etc/redhat-release"): with open("/etc/redhat-release", "r") as f: content = f.read() match = re.search(r"release\s+(\d+)", content, re.IGNORECASE) if match: version = int(match.group(1)) self.rhel_version = version return version - except Exception: - pass + except Exception as e: + self.log(f"Failed to detect RHEL version: {e}") # Default to RHEL 9 self.rhel_version = 9 return 9
343-348: Hardcoded screenshot path creates fragile dependency.The path
/home/desktop.pngis hardcoded assuming thegvncviewer-customizetool always saves to this location. Consider parameterizing this or documenting the dependency on tool behavior.Proposed improvement
+ # gvncviewer-customize saves to /home/desktop.png by default + # This path is determined by the tool's implementation + default_screenshot = self.params.get( + "gvncviewer_screenshot_path", "/home/desktop.png" + ) - # gvncviewer-customize saves to /home/desktop.png by default - default_screenshot = "/home/desktop.png"
712-713: Hardcoded settle time could be parameterized.The 5-second settle time is hardcoded. Consider making this configurable via params for different guest performance characteristics.
Proposed improvement
- settle_time = 5 - time.sleep(settle_time) + settle_time = int(self.params.get("visual_settle_time", 5)) + time.sleep(settle_time)
816-825: Method name is misleading - it also restarts the VM.The method
_wait_for_shutdownnot only waits for shutdown but also restarts the VM (line 824). Consider renaming to_wait_for_shutdown_and_restartor_perform_powerdown_cycleto accurately reflect its behavior.
1333-1357: Usetest.error()ortest.cancel()instead oftest.fail()for unimplemented tests.The stub methods use
self.test.fail()for not-yet-implemented functionality. Semantically:
test.fail()= test ran but assertions failed (bug found)test.error()= test could not run due to environment/setup issuestest.cancel()= test intentionally skippedFor unimplemented features,
test.cancel()with a message ortest.error()would be more appropriate to distinguish from actual test failures.Proposed fix (example for one method)
@error_context.context_aware def run_check_driver_signature_signtool(self): - self.test.fail( + self.test.cancel( "Test action 'check_driver_signature_signtool' is not yet implemented." )
1168-1171: Session list grows unboundedly during reboot loops.Each reboot appends a new session to
_open_session_listwithout removing the old one. While_cleanup_open_sessionhandles this correctly at the end, during long-running tests with many reboots, the list accumulates stale session references. Consider closing the old session before appending the new one, or clearing the list after successful reboot.Proposed improvement
for i in range(5): error_context.context("Reboot loop %d/5" % (i + 1), LOG_JOB.info) + # Close current session before reboot + if self.session: + try: + self.session.close() + except Exception: + pass self.session = self.vm.reboot( session=self.session, method="shell", timeout=reboot_timeout ) self._open_session_list.append(self.session)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_vga_visual_test.cfgqemu/tests/virtio_vga_visual_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_vga_visual_test.py (1)
provider/win_driver_utils.py (1)
install_driver_by_virtio_media(159-194)
🔇 Additional comments (8)
qemu/tests/cfg/virtio_vga_visual_test.cfg (1)
1-51: Configuration looks well-structured.The test configuration is comprehensive with appropriate timeouts, command templates, and AI prompts. A few observations:
Line 46: The
gvncviewer_tool_urlpoints to an internal Red Hat server. Ensure this URL is accessible from the test environment and consider documenting fallback options for external contributors.Line 11: The
device_hwiduses a specific hardware ID with revision. Verify this matches all supported virtio-vga device variants.qemu/tests/virtio_vga_visual_test.py (7)
424-471: LGTM - Dual verification approach is well-designed.The method correctly implements a fallback strategy: try VNC first, always capture internal screenshot as diagnostic backup, and clearly differentiates between VNC transmission issues and actual driver problems. The return tuple provides all necessary context for callers.
936-948: Regex parsing for driver blocks looks correct.The regex pattern correctly handles both Windows 2016 and newer pnputil output formats. The use of
re.DOTALLensures multi-line block matching works properly.
1424-1448: Good multi-layered verification approach for resolution testing.The implementation correctly verifies resolution through three independent methods:
- Command-line query from guest OS
- Screenshot file dimensions analysis
- AI-based visual quality check
The 10-pixel tolerance (line 1436) is reasonable for minor rendering differences.
1497-1527: Clean dispatcher pattern for test execution.The dynamic method dispatch using
getattris a clean approach that scales well with new test types. The try/finally block ensures session cleanup even on failures.
879-890: LGTM - Clean path resolution utility.The
_resolve_winutils_pathmethod correctly handles the WIN_UTILS: placeholder resolution with appropriate error handling when the drive letter cannot be detected.
1206-1250: Well-designed install/uninstall test workflow.The test correctly:
- Ejects the virtio ISO before uninstalling to prevent auto-reinstall
- Verifies the guest state after uninstallation
- Re-inserts the ISO and reinstalls
- Includes reboots at appropriate points
This ensures proper test isolation and verification at each stage.
103-112: No issue found. Thexwfb-runbinary check is correct—it is one of the binaries provided by thexwayland-runpackage (along withxwayland-runandwlheadless-run). The code logic is sound: it checks ifxwfb-runis available, and if not, installs thexwayland-runpackage to provide it.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@qemu/tests/virtio_vga_visual_test.py`:
- Around line 676-690: The Gemini yes/no parsing using response.lower().split()
can miss punctuation like "NO." or "YES,", so update the checks that examine
response and internal_response in virtio_vga_visual_test.py (the blocks around
prompt_desktop, response = ppm_utils.verify_screen_with_gemini and
internal_response = ppm_utils.verify_screen_with_gemini) to normalize and
robustly test for whole-word yes/no (e.g., lowercase the string, strip
surrounding punctuation, or use a word-boundary regex like r'\b(?:yes|no)\b' to
search for matches) and apply the same change to the other occurrences noted
(around the blocks at ~760-788 and ~1493-1499) so punctuation-wrapped answers
are recognized correctly.
- Around line 1074-1091: wmic.parse_list() can return a string which will make
the for loop iterate characters; before iterating inf_names (the variable from
wmic.parse_list) add the same defensive type check used earlier: detect if
inf_names is a str and convert it into a single-element list (or an empty list
if it's falsy) so the subsequent loop over pnputil_delete_cmd and the
session.cmd_status_output calls receive whole INF names rather than characters;
update the block that sets inf_names and before the for inf_name in inf_names
loop perform this type normalization.
- Around line 140-191: The download_tool currently fetches gvncviewer from an
HTTP URL (tool_url) without integrity checks, creating a supply-chain risk;
update download_tool to require or accept a new gvncviewer_tool_sha256 test
parameter and after download compute the file's SHA256 (using
tool_download_path) and compare it to the provided value, raising RuntimeError
on mismatch, and also reject non-HTTPS URLs (tool_url) unless a checksum is
provided; update install_tool to rely on the verified download and ensure the
same symbols (download_tool, install_tool, tool_url, tool_download_path,
tool_install_path, gvncviewer_tool_sha256) are used so existing flows call the
verified path.
| def download_tool(self): | ||
| """Download gvncviewer-customize from server.""" | ||
| if not self.tool_url: | ||
| raise ValueError("gvncviewer_tool_url not specified in test parameters") | ||
|
|
||
| # Remove old download | ||
| if os.path.exists(self.tool_download_path): | ||
| os.remove(self.tool_download_path) | ||
|
|
||
| # Use curl or wget | ||
| rc, _, _ = self._run_cmd(["which", "curl"], check=False) | ||
| if rc == 0: | ||
| self._run_cmd( | ||
| ["curl", "-L", "-o", self.tool_download_path, self.tool_url], | ||
| timeout=120, | ||
| ) | ||
| else: | ||
| self._run_cmd( | ||
| ["wget", "-O", self.tool_download_path, self.tool_url], timeout=120 | ||
| ) | ||
|
|
||
| # Verify download | ||
| if ( | ||
| not os.path.exists(self.tool_download_path) | ||
| or os.path.getsize(self.tool_download_path) == 0 | ||
| ): | ||
| raise RuntimeError("Tool download failed") | ||
|
|
||
| return self.tool_download_path | ||
|
|
||
| def install_tool(self): | ||
| """Install gvncviewer-customize to /usr/bin.""" | ||
| # Check if already installed and executable | ||
| if os.path.exists(self.tool_install_path) and os.access( | ||
| self.tool_install_path, os.X_OK | ||
| ): | ||
| return True | ||
|
|
||
| # Download if needed | ||
| if not os.path.exists(self.tool_download_path): | ||
| self.download_tool() | ||
|
|
||
| # Make executable and move to /usr/bin | ||
| os.chmod(self.tool_download_path, 0o755) | ||
| self._run_cmd(["mv", self.tool_download_path, self.tool_install_path]) | ||
|
|
||
| # Verify installation | ||
| if not os.path.exists(self.tool_install_path) or not os.access( | ||
| self.tool_install_path, os.X_OK | ||
| ): | ||
| raise RuntimeError("Tool installation failed") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing checksum/verification patterns and gvncviewer references
rg -n --type=py --type=cfg 'sha256|checksum|verify.*download|gvncviewer' qemu/testsRepository: autotest/tp-qemu
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for tool_url and tool_download_path patterns
rg -n 'tool_url|tool_download_path|tool_install_path' qemu/tests --type=pyRepository: autotest/tp-qemu
Length of output: 2131
🏁 Script executed:
#!/bin/bash
# Check if params are used for tool configuration elsewhere
rg -n 'self\.params\.get.*tool|self\.params.*download' qemu/tests --type=py -A 2Repository: autotest/tp-qemu
Length of output: 384
🏁 Script executed:
#!/bin/bash
# Look for HTTPS enforcement or SSL/TLS patterns in downloads
rg -n 'https://|http://|verify.*cert|ssl|tls' qemu/tests --type=py | head -30Repository: autotest/tp-qemu
Length of output: 2841
🏁 Script executed:
#!/bin/bash
# Check the full context around tool initialization and params
sed -n '1,50p' qemu/tests/virtio_vga_visual_test.pyRepository: autotest/tp-qemu
Length of output: 1587
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about gvncviewer_tool_url in the repo
rg -n 'gvncviewer_tool_url' -B 3 -A 3 qemu/tests --type=pyRepository: autotest/tp-qemu
Length of output: 1180
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or test config files mentioning this
find qemu -name "*.cfg" -o -name "*.conf" -o -name "README*" -o -name "*.md" | xargs grep -l "gvncviewer" 2>/dev/null || echo "No config files found"Repository: autotest/tp-qemu
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check if there's any checksum/hash validation patterns used in downloads anywhere in the codebase
rg -n 'hashlib|sha256|sha1|md5' qemu/tests --type=py | head -20Repository: autotest/tp-qemu
Length of output: 1972
🏁 Script executed:
#!/bin/bash
# Check the test config file for parameter documentation
cat qemu/tests/cfg/virtio_vga_visual_test.cfgRepository: autotest/tp-qemu
Length of output: 4991
🏁 Script executed:
#!/bin/bash
# Check if other similar test files use checksum parameters
rg -n 'tool_sha256|tool_checksum|download.*sha256' qemu/tests --type=pyRepository: autotest/tp-qemu
Length of output: 42
Add integrity verification for the downloaded VNC tool.
The executable is downloaded from an HTTP URL (http://download.eng.pek2.redhat.com/...) without checksum or signature validation, creating a supply-chain risk. Add a checksum parameter and either require it or enforce HTTPS.
Suggested hardening (checksum + non-HTTPS guard)
+import hashlib
+
def download_tool(self):
"""Download gvncviewer-customize from server."""
if not self.tool_url:
raise ValueError("gvncviewer_tool_url not specified in test parameters")
@@
# Verify download
if (
not os.path.exists(self.tool_download_path)
or os.path.getsize(self.tool_download_path) == 0
):
raise RuntimeError("Tool download failed")
+
+ expected_sha256 = self.params.get("gvncviewer_tool_sha256")
+ if expected_sha256:
+ with open(self.tool_download_path, "rb") as f:
+ actual = hashlib.sha256(f.read()).hexdigest()
+ if actual.lower() != expected_sha256.lower():
+ raise RuntimeError("Tool checksum mismatch")
+ elif not self.tool_url.startswith("https://"):
+ raise RuntimeError("Refusing non-HTTPS download without checksum")Obtain or compute the SHA256 of the gvncviewer-customize tool and add gvncviewer_tool_sha256 to the test config, or migrate the URL to HTTPS.
🤖 Prompt for AI Agents
In `@qemu/tests/virtio_vga_visual_test.py` around lines 140 - 191, The
download_tool currently fetches gvncviewer from an HTTP URL (tool_url) without
integrity checks, creating a supply-chain risk; update download_tool to require
or accept a new gvncviewer_tool_sha256 test parameter and after download compute
the file's SHA256 (using tool_download_path) and compare it to the provided
value, raising RuntimeError on mismatch, and also reject non-HTTPS URLs
(tool_url) unless a checksum is provided; update install_tool to rely on the
verified download and ensure the same symbols (download_tool, install_tool,
tool_url, tool_download_path, tool_install_path, gvncviewer_tool_sha256) are
used so existing flows call the verified path.
| prompt_desktop = self.params.get("prompt_desktop_check") | ||
| response = ppm_utils.verify_screen_with_gemini( | ||
| screenshot_path, prompt_desktop, results_dir=self.results_dir | ||
| ) | ||
|
|
||
| if "no" in response.lower().split(): | ||
| # Primary verification failed, need to diagnose | ||
| if vnc_success and internal_success: | ||
| # VNC verification failed, check Internal as diagnostic | ||
| internal_response = ppm_utils.verify_screen_with_gemini( | ||
| internal_path, prompt_desktop, results_dir=self.results_dir | ||
| ) | ||
|
|
||
| if "yes" in internal_response.lower().split(): | ||
| # Internal is OK, VNC has display issue - ERROR |
There was a problem hiding this comment.
Normalize Gemini yes/no parsing to handle punctuation.
response.lower().split() misses values like NO. or YES,, which can silently misclassify results. Use a word‑boundary regex for robust parsing.
✅ Robust yes/no parsing
- if "no" in response.lower().split():
+ if re.search(r"\bno\b", response, re.I):
@@
- if "yes" in internal_response.lower().split():
+ if re.search(r"\byes\b", internal_response, re.I):
@@
- if "yes" in resp.lower().split():
+ if re.search(r"\byes\b", resp, re.I):
@@
- if "yes" in internal_response.lower().split():
+ if re.search(r"\byes\b", internal_response, re.I):
@@
- if "no" in ai_response.lower().split():
+ if re.search(r"\bno\b", ai_response, re.I):Also applies to: 760-788, 1493-1499
🤖 Prompt for AI Agents
In `@qemu/tests/virtio_vga_visual_test.py` around lines 676 - 690, The Gemini
yes/no parsing using response.lower().split() can miss punctuation like "NO." or
"YES,", so update the checks that examine response and internal_response in
virtio_vga_visual_test.py (the blocks around prompt_desktop, response =
ppm_utils.verify_screen_with_gemini and internal_response =
ppm_utils.verify_screen_with_gemini) to normalize and robustly test for
whole-word yes/no (e.g., lowercase the string, strip surrounding punctuation, or
use a word-boundary regex like r'\b(?:yes|no)\b' to search for matches) and
apply the same change to the other occurrences noted (around the blocks at
~760-788 and ~1493-1499) so punctuation-wrapped answers are recognized
correctly.
| output = self.session.cmd(cmd, timeout=360) | ||
| inf_names = wmic.parse_list(output) | ||
|
|
||
| if not inf_names: | ||
| LOG_JOB.warning("No driver packages found for '%s'", device_name) | ||
| else: | ||
| LOG_JOB.info("Uninstalling %d driver package(s)", len(inf_names)) | ||
|
|
||
| cmd_key = ( | ||
| "pnputil_delete_cmd_win2016" if self._is_win2016() else "pnputil_delete_cmd" | ||
| ) | ||
| pnputil_delete_cmd = self.params.get(cmd_key) | ||
|
|
||
| for inf_name in inf_names: | ||
| status, output = self.session.cmd_status_output( | ||
| pnputil_delete_cmd % inf_name, inst_timeout | ||
| ) | ||
| if status not in (0, 3010): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find wmic module and parse_list function
find . -name "*wmic*.py" -type f | head -20Repository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for parse_list definition
rg -n "def parse_list" --type=pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Get context around parse_list implementation
rg -n -A 20 "def parse_list" --type=pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for parse_list across all Python files
rg -n "parse_list" --type=py -A 5Repository: autotest/tp-qemu
Length of output: 2926
🏁 Script executed:
#!/bin/bash
# Look at the imports and structure of virtio_vga_visual_test.py
head -100 qemu/tests/virtio_vga_visual_test.py | grep -E "^import|^from|wmic"Repository: autotest/tp-qemu
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Search for wmic import or usage
rg -n "wmic" --type=py -B 2 -A 2Repository: autotest/tp-qemu
Length of output: 24711
🏁 Script executed:
#!/bin/bash
# Find the wmic module in virttest
find . -path "*/virttest/utils_windows*" -name "*wmic*" -type fRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for parse_list in virttest utilities
rg -n "def parse_list" --type=pyRepository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for utils_windows directory
find . -type d -name "utils_windows"Repository: autotest/tp-qemu
Length of output: 42
Add type validation for wmic.parse_list() output before iteration.
The code at lines 986-988 in this same file already includes a defensive check for this scenario. Apply the same pattern here: if parse_list() returns a string instead of a list, the for loop will iterate character-by-character, passing invalid inf names to pnputil.
Suggested fix
inf_names = wmic.parse_list(output)
if not inf_names:
LOG_JOB.warning("No driver packages found for '%s'", device_name)
else:
+ if not isinstance(inf_names, list):
+ inf_names = [inf_names]
LOG_JOB.info("Uninstalling %d driver package(s)", len(inf_names))🤖 Prompt for AI Agents
In `@qemu/tests/virtio_vga_visual_test.py` around lines 1074 - 1091,
wmic.parse_list() can return a string which will make the for loop iterate
characters; before iterating inf_names (the variable from wmic.parse_list) add
the same defensive type check used earlier: detect if inf_names is a str and
convert it into a single-element list (or an empty list if it's falsy) so the
subsequent loop over pnputil_delete_cmd and the session.cmd_status_output calls
receive whole INF names rather than characters; update the block that sets
inf_names and before the for inf_name in inf_names loop perform this type
normalization.
6-dehan
force-pushed
the
gpu-AI-image
branch
2 times, most recently
from
c28d5f9 to
f52cbc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@qemu/tests/virtio_vga_visual_test.py`:
- Around line 115-119: The current check treats any existing lock_file
("/tmp/.X%s-lock" % self.display_num) as proof the display is running; instead,
read the PID from that lock_file (lock_file), verify the process is alive (e.g.,
os.kill(pid, 0) or equivalent) and only then set os.environ["DISPLAY"]=display
and return True; if the PID is not running or the file contains invalid data,
remove the stale lock_file and continue (do not return True). Update the logic
in the method that uses self.display_num and display to perform this liveness
check and cleanup before assuming the display is active.
♻️ Duplicate comments (9)
qemu/tests/virtio_vga_visual_test.py (9)
99-108: Typo in xwayland command check (previously flagged).Line 101 checks for
xwfb-runbut should check forxwayland-runto match the installed package.
140-168: Supply-chain risk: No checksum verification for downloaded tool (previously flagged).The download lacks integrity verification. Previous review suggested adding SHA256 checksum validation.
283-295: VNC environment setup with idempotent pattern (Gemini validation previously flagged).The one-time setup pattern is correct. A previous review suggested adding early Gemini API validation here to fail fast if credentials are misconfigured.
314-327: Malformed xwayland command construction (previously flagged).The
-sflag syntax and-geometryoption usage were flagged in a previous review as invalid for xwfb-run/mutter.
423-425: Potential TypeError if delete_file_cmd is missing (previously flagged).If
delete_file_cmdis not in params, the%formatting will raise TypeError on None.
589-596: Device name truncation may cause false positives (previously flagged).The check uses
device_name[0:30]which could match wrong drivers if they share a prefix.
681-689: Gemini yes/no parsing may miss punctuated responses (previously flagged).Using
response.lower().split()won't match "NO." or "YES," due to punctuation. A previous review suggested using word-boundary regex.
1074-1091: Missing type validation for wmic.parse_list() output (previously flagged).Unlike
_clean_driver_store(which has the fix at lines 986-988), this method doesn't validate thatinf_namesis a list before iterating.
1225-1228: Missing session parameter in reboot call (previously flagged).The
vm.reboot()call here doesn't passsession=self.sessionunlike the similar call inrun_reboot_with_driverat line 1200-1201.
🧹 Nitpick comments (3)
qemu/tests/virtio_vga_visual_test.py (3)
61-77: Consider logging when defaulting to RHEL version.The fallback to RHEL 9 is reasonable, but silently defaulting could mask detection issues. Consider adding a debug log when the default is used.
Suggested improvement
except Exception: pass # Default to RHEL 9 self.rhel_version = 9 + LOG_JOB.debug("Could not detect RHEL version, defaulting to 9") return 9
348-354: Hardcoded screenshot path reduces flexibility.The default screenshot path
/home/desktop.pngis hardcoded. Consider making this configurable via params or deriving it from the tool's behavior, in case the tool's default changes or varies by version.Suggested improvement
+ # gvncviewer-customize saves to /home/desktop.png by default + default_screenshot = self.params.get( + "gvncviewer_default_output", "/home/desktop.png" + ) - # gvncviewer-customize saves to /home/desktop.png by default - default_screenshot = "/home/desktop.png"
1369-1393: Consider using test.cancel() for unimplemented stubs.Using
test.fail()for unimplemented test actions marks them as failures. Consider usingtest.cancel()(if available) or a skip mechanism to indicate these are planned but not yet implemented, rather than actual test failures.Suggested change (example)
`@error_context.context_aware` def run_check_driver_signature_signtool(self): - self.test.fail( + self.test.cancel( "Test action 'check_driver_signature_signtool' is not yet implemented." )
| # Check if display already running by checking lock file | ||
| lock_file = "/tmp/.X%s-lock" % self.display_num | ||
| if os.path.exists(lock_file): | ||
| os.environ["DISPLAY"] = display | ||
| return True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file structure and search for process/pid verification patterns
rg -n "proc|pid|stale|lock" qemu/tests/virtio_vga_visual_test.py -A 2 -B 2Repository: autotest/tp-qemu
Length of output: 3293
🏁 Script executed:
# Get the full start_xvfb method to see current implementation
sed -n '110,138p' qemu/tests/virtio_vga_visual_test.pyRepository: autotest/tp-qemu
Length of output: 1058
🏁 Script executed:
# Check git log for commit c28d5f9 or related commits mentioning lock or stale
git log --oneline --all --grep="lock\|stale" 2>/dev/null | head -20Repository: autotest/tp-qemu
Length of output: 42
🏁 Script executed:
# Search for any process checking logic in the file
ast-grep --pattern 'Popen|subprocess|psutil|os.kill|os.path.isfile' qemu/tests/virtio_vga_visual_test.pyRepository: autotest/tp-qemu
Length of output: 42
Address stale lock file vulnerability in display check.
The code at lines 117-119 assumes a lock file's existence means the display is running, but does not verify the process is actually alive. This can cause silent failures if the lock file persists after an Xvfb process crashes. The previous review reference to commit c28d5f9 cannot be found in the repository history. Add process liveness verification when a lock file exists—either by checking if the process is still running or by attempting to verify display connectivity before returning True.
🤖 Prompt for AI Agents
In `@qemu/tests/virtio_vga_visual_test.py` around lines 115 - 119, The current
check treats any existing lock_file ("/tmp/.X%s-lock" % self.display_num) as
proof the display is running; instead, read the PID from that lock_file
(lock_file), verify the process is alive (e.g., os.kill(pid, 0) or equivalent)
and only then set os.environ["DISPLAY"]=display and return True; if the PID is
not running or the file contains invalid data, remove the stale lock_file and
continue (do not return True). Update the logic in the method that uses
self.display_num and display to perform this liveness check and cleanup before
assuming the display is active.
Introduces a comprehensive visual testing framework for virtio-vga driver validation using AI (Gemini) to verify screen states and display behavior. Key features: - VNC environment manager with Xvfb/Xwayland support for RHEL 8-10 - Dual screenshot capture (VNC + internal guest) - AI-powered visual verification for desktop state, Start Menu, BSOD detection - Multiple test scenarios: driver install/uninstall, resolution changes, shutdown/reboot/reset testing, driver upgrade/downgrade - Diagnostic capabilities to distinguish VNC vs driver issues Signed-off-by: Dehan Meng <[email protected]>
add 'virtio_vga_visual_test.check_driver_signature_signtool' and 'virtio_vga_visual_test.check_driver_signature_sigverif' two cases Signed-off-by: Dehan Meng <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@qemu/tests/virtio_vga_visual_test.py`:
- Around line 737-740: The call to self.session.cmd(cmd) using the
start_menu_cmd may raise a timeout/command error but is not handled, causing a
misleading "Start Menu did not open" failure; wrap the invocation of
start_menu_cmd (the variable start_menu_cmd) and the call to
self.session.cmd(...) in a try/except (or check the command return/status if the
session API provides one), log or raise a clear test failure on command
execution error, and only proceed to the time.sleep(3) and subsequent menu
checks if the command succeeded (use the same self.session object and the
surrounding block that currently calls session.cmd and time.sleep to implement
this guard).
♻️ Duplicate comments (2)
qemu/tests/virtio_vga_visual_test.py (2)
1309-1312: Inconsistent reboot call - missingsessionparameter.The
vm.reboot()call at line 1311 doesn't pass thesessionparameter, unlike the similar call inrun_reboot_with_driver(lines 1284-1286). This could cause issues with session tracking during system resets.🔎 Proposed fix
for i in range(5): error_context.context("Reset loop %d/5" % (i + 1), LOG_JOB.info) - self.session = self.vm.reboot(method="system_reset", timeout=reboot_timeout) + self.session = self.vm.reboot( + session=self.session, method="system_reset", timeout=reboot_timeout + ) self._open_session_list.append(self.session)
203-209: RHEL 10+ display server not started after installation.For RHEL 10+,
install_xwayland()is called but no equivalent ofstart_xvfb()is invoked. The xwayland display won't be running when VNC screenshots are attempted, causing failures on RHEL 10+ hosts.🔎 Proposed fix
# Install display server based on RHEL version if version >= 10: # RHEL 10+: Use xwayland-run self.install_xwayland() + # Note: xwayland-run is invoked per-command in _vnc_screendump() + # No persistent display server needed for this approach else: # RHEL 8/9: Use Xvfb self.install_xvfb() self.start_xvfb()If xwayland-run is indeed meant to wrap each command (as done in
_vnc_screendump), add a comment clarifying this design. Otherwise, implement astart_xwayland()method similar tostart_xvfb().
| cmd = self.params.get("start_menu_cmd") | ||
| self.session.cmd(cmd) | ||
|
|
||
| time.sleep(3) |
There was a problem hiding this comment.
Missing error handling for Start Menu invocation command.
If the start_menu_cmd fails (e.g., session timeout, command error), the test will proceed to check for menu visibility and fail with a misleading "Start Menu did not open" error instead of indicating the command execution failed.
🔎 Proposed fix
prompt_start_menu = self.params.get("prompt_start_menu_check")
cmd = self.params.get("start_menu_cmd")
- self.session.cmd(cmd)
+ try:
+ self.session.cmd(cmd, timeout=30)
+ except Exception as e:
+ LOG_JOB.warning("Failed to execute start menu command: %s", e)
time.sleep(3)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmd = self.params.get("start_menu_cmd") | |
| self.session.cmd(cmd) | |
| time.sleep(3) | |
| cmd = self.params.get("start_menu_cmd") | |
| try: | |
| self.session.cmd(cmd, timeout=30) | |
| except Exception as e: | |
| LOG_JOB.warning("Failed to execute start menu command: %s", e) | |
| time.sleep(3) |
🤖 Prompt for AI Agents
In `@qemu/tests/virtio_vga_visual_test.py` around lines 737 - 740, The call to
self.session.cmd(cmd) using the start_menu_cmd may raise a timeout/command error
but is not handled, causing a misleading "Start Menu did not open" failure; wrap
the invocation of start_menu_cmd (the variable start_menu_cmd) and the call to
self.session.cmd(...) in a try/except (or check the command return/status if the
session API provides one), log or raise a clear test failure on command
execution error, and only proceed to the time.sleep(3) and subsequent menu
checks if the command succeeded (use the same self.session object and the
surrounding block that currently calls session.cmd and time.sleep to implement
this guard).
1 participant
Introduces a comprehensive visual testing framework for virtio-vga driver validation using AI (Gemini) to verify screen states and display behavior.
Key features:
ID: 4900
Signed-off-by: Dehan Meng [email protected]
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.