Virt-Manager review support#37
Merged
arighi merged 2 commits intoJul 23, 2026
Merged
Conversation
NathanChenNVIDIA
force-pushed
the
virt-manager-review-support
branch
from
July 21, 2026 16:34
6255b95 to
8d90f24
Compare
arighi
approved these changes
Jul 22, 2026
arighi
left a comment
arighi
left a comment
Collaborator
There was a problem hiding this comment.
Left two comments (based on codex feedback), let me know if they make sense. Other than that everything LGTM, thanks!
| and release them when the object is cleaned up (subclass `_cleanup()`). A | ||
| handle created outside these wrappers — a raw `GLib.timeout_add`, or a | ||
| connection on a longer-lived object that outlives this one — leaks and can fire | ||
| a callback on a destroyed widget. |
Collaborator
There was a problem hiding this comment.
codex suggests to rephrase this part to something along these lines:
- `vmmGObject` subclasses must chain the base `__init__`. Its `connect()` /
`connect_once()` / `connect_opt_out()` and `timeout_add()` helpers track their
handles for cleanup.
- `vmmGObject.idle_add()` is a thin wrapper around `GLib.idle_add()`; it does
not register the source ID for object cleanup. An idle callback normally
removes itself after it returns `False`/`None`, so using `idle_add()` is not
inherently a leak. Check whether the callback can remain pending until after
object teardown, retain a dead object, or return `True` and repeat.
- Raw `GLib.timeout_add()` / `GLib.idle_add()` and signal connections are valid
when their handle is explicitly removed or their lifetime is otherwise
bounded. Report a bug only when you can show that the source or connection
outlives its owner and can invoke stale state.
- Pay particular attention to handlers connected on a longer-lived emitter:
they can retain the receiver and must be disconnected when the receiver is
cleaned up.
| - **`vmmGObject` cleanup**: connections and timers made through the tracked | ||
| wrappers (`connect()` / `connect_once()` / `connect_opt_out()`, | ||
| `timeout_add()`, `idle_add()`) are released automatically when the object is | ||
| cleaned up. Don't claim a leak for a handle that goes through the tracked path. |
Collaborator
There was a problem hiding this comment.
...and here:
- **GObject source cleanup**: `vmmGObject` tracks signal connections made
through its connection helpers and timers made through `timeout_add()`.
`idle_add()` is not tracked for cleanup, but its callback normally removes
itself after returning `False`/`None`. Do not report a leak merely because a
raw or wrapped GLib source is used. Trace whether it is self-removing,
explicitly removed, or can remain active after its owning object is cleaned
up. Keep a finding only when the surviving source has a concrete stale-object
or retention consequence.
Collaborator
Author
There was a problem hiding this comment.
Thanks for the review, I've incorporated these changes now!
Add the boro-authored virt-manager prompt corpus under
resources/prompts/virt-manager/ (core patterns, coding style, severity,
false-positive guide, callstack, inline-template and per-subsystem
guides for virtinst / virtManager), mirroring the qemu and libvirt
corpus layout plus the resources/prompts/virt-manager.local/ overlay
directory.
Symbols and APIs in the always-loaded guides are verified against the
current upstream virt-manager tree: the tracked vmmGObject wrappers are
connect() / connect_once() / connect_opt_out() / timeout_add() /
idle_add() (there is no connect_* helper family); domain polling is
vmmDomain.tick(); install is driven by Installer.start_install(guest);
Guest.set_defaults() adds default devices via its _add_default_*
helpers; GTK is gi.require_version("Gtk", "3.0"); and code logs via the
module log (from virtinst import log).
Also add virt-manager overrides for the shared review prompts consumed
by the target in the following commit: a fast-mode one-shot, a
false-positive digest, and the specialist stage bodies (execution,
resource, locking, security, portability, comments), written in
Python/GTK terms (virtinst XMLBuilder, GObject/GTK main-thread rules,
gi.require_version) with no kernel-only mandates.
Signed-off-by: Nathan Chen <nathanc@nvidia.com>
Register virt-manager as a fourth review target, following the modular src/target/ layout. Add src/target/virt_manager.rs implementing TargetSpec over the embedded resources/prompts/virt-manager/ corpus (subsystem map, core files, and virt-manager-specific reviewer personas), and wire it through ReviewTarget::VirtManager, the --target virt-manager CLI value, and best-effort tree detection. Make the shared discovery/validation pipeline target-aware: virt-manager overrides one_shot_review(), false_positive_digest() and stage_instructions() (the TargetSpec methods added with the libvirt target) so fast mode, the false-positive digest and the specialist stages use Python/GTK guidance instead of the kernel corpus. Add a test asserting the fully assembled virt-manager payload carries no kernel-only tokens. Signed-off-by: Nathan Chen <nathanc@nvidia.com>
NathanChenNVIDIA
force-pushed
the
virt-manager-review-support
branch
from
July 22, 2026 19:27
8d90f24 to
006245f
Compare
arighi
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for virt-manager reviews, registering virt-manager as a fourth review target alongside kernel, QEMU, and Libvirt.
It follows the existing modular src/target/ layout (a TargetSpec implementation in src/target/virt_manager.rs over an embedded resources/prompts/virt-manager/ corpus). boro review now accepts --target virt-manager, which selects the matching prompt corpus and reviewer persona, and warns when the selected target appears to mismatch the source tree.