Libvirt review support#23
Conversation
Add the boro-authored libvirt prompt corpus under resources/prompts/libvirt/ (core patterns, coding style, severity, false-positive guide, callstack, inline-template and per-subsystem guides), mirroring the qemu corpus layout. Also add the resources/prompts/libvirt.local/ overlay directory and the review-validation-libvirt.md parity file. These resources are consumed by the libvirt review target added in the following commit. Signed-off-by: Nathan Chen <nathanc@nvidia.com>
Register libvirt as a third review target alongside kernel and QEMU, following the modular src/target/ layout. Add src/target/libvirt.rs implementing TargetSpec over the embedded resources/prompts/libvirt/ corpus (subsystem map, core files, and libvirt-specific reviewer, phase-0, mailing-list, second-opinion and quick-summary personas), and wire it through ReviewTarget::Libvirt, the --target CLI value, and best-effort tree detection. Signed-off-by: Nathan Chen <nathanc@nvidia.com>
NathanChenNVIDIA
force-pushed
the
libvirt-review-support
branch
from
a14f225 to
1d99b85
Compare
nirmoy
left a comment
nirmoy
left a comment
There was a problem hiding this comment.
Codex found these issues during review of PR #23. I am leaving six actionable inline comments below. The Rust implementation and tests pass; the issues are in target-wide review plumbing and the technical accuracy of the new Libvirt prompt corpus.
| "embedded resources/prompts/libvirt (baked into binary at build time)" | ||
| } | ||
|
|
||
| fn reviewer_system_prompt(&self) -> &'static str { |
There was a problem hiding this comment.
[P1] Targetize the entire review pipeline
Codex found that the Libvirt-specific implementation stops at corpus/persona selection. Fast mode still injects the kernel-only one-shot prompt; normal mode additionally uses kernel specialist stages, the kernel false-positive digest, and the kernel findings validator, while review-validation-libvirt.md is never referenced. This gives the model contradictory Kconfig/Kbuild, GFP_KERNEL, RCU, copy_to_user, and DMA requirements. Please make the shared discovery/validation prompts target-aware or domain-neutral and add a test over the fully assembled Libvirt payload.
| match self { | ||
| TargetArg::Kernel => config::ReviewTarget::Kernel, | ||
| TargetArg::Qemu => config::ReviewTarget::Qemu, | ||
| TargetArg::Libvirt => config::ReviewTarget::Libvirt, |
There was a problem hiding this comment.
[P2] Do not query kernel lore for Libvirt reviews
Codex found that this target reaches run_two_pass(), where lore_active ignores ReviewTarget. With lei installed and the default enabled configuration, a Libvirt review queries https://lore.kernel.org/all/ using a Linux-kernel follow-up prompt. A generic Libvirt subject can therefore inject unrelated kernel discussion into the review context. Please gate this stage to ReviewTarget::Kernel or provide a Libvirt-specific archive and prompt.
| - **Domain object lifetime**: a `virDomainObj` is obtained locked+ref'd | ||
| (`qemuDomObjFromDomain` / `virDomainObjListFindBy*`) and released with | ||
| `virDomainObjEndAPI(&vm)` on all paths. Pair every lookup with an EndAPI. | ||
| - **Jobs**: modifying a domain requires `qemuDomainObjBeginJob()` with the right |
There was a problem hiding this comment.
[P2] Use the current domain-job APIs
Codex found that current upstream Libvirt uses virDomainObjBeginJob(), virDomainObjEndJob(), and VIR_JOB_*; qemuDomainObjBeginJob(), qemuDomainObjEndJob(), and QEMU_JOB_MODIFY do not exist. The always-loaded locking guide repeats these stale names, so correct code can be misdiagnosed and suggested fixes will not compile. Please update every occurrence in the new corpus.
| - Data read back from `/proc`, sysfs, or hypervisor logs. | ||
|
|
||
| For each: read a field once into a local, validate it (range, enum, length, | ||
| NULL), and only then use it. `virStrToLong_*` / `virStrToDouble` return -1 on |
There was a problem hiding this comment.
[P2] Describe suffix parsing correctly
Codex found that virStrToLong_* and virStrToDouble reject trailing bytes only when end_ptr == NULL. With a non-NULL end pointer they intentionally succeed and leave suffix validation to the caller. This always-loaded statement can therefore hide real suffix-validation bugs. Please qualify the rule and replace the nonexistent virStrToUll name in the other guides with virStrToLong_ull.
| - `virBuffer` builds strings incrementally (`virBufferAsprintf`, | ||
| `virBufferAddLit`, `virBufferEscapeString`). Always emit XML/shell content | ||
| through the escaping helpers, and remember the auto-cleanup | ||
| (`g_auto(virBuffer)`); reading a buffer that hit an error returns NULL. |
There was a problem hiding this comment.
[P2] Remove the nonexistent virBuffer error state
Codex found that current virBuffer contains only a GString * and indentation. virBufferCurrentContent() returns NULL only when the buffer pointer itself is NULL and returns an empty string for an empty buffer; there is no allocation-error state to check. This guidance will produce false positives at normal call sites. Please remove it here and from subsystem/util.md.
| - `virObjectLockable` objects are locked with `virObjectLock` / | ||
| `virObjectUnlock` (RWLockable: `virObjectRWLockRead`/`Write`). A bare struct | ||
| field touched from two threads without its lock is a data race. | ||
| - **Lock ordering is the classic deadlock source.** The established order is |
There was a problem hiding this comment.
[P2] Avoid universal lock and monitor rules
Codex found that this conflates the driver configuration mutex with the domain-list lock and later mandates an active check after every monitor exit. Upstream intentionally takes driver->lock through virQEMUDriverGetConfig() while a domain can be locked, and hotplug paths update vm->def immediately after qemuDomainObjExitMonitor() under the domain job. Please require proof of an actual inverse lock edge or unsafe stale-state use instead; the current universal rules also conflict with the new false-positive guide.
2 participants
This PR adds support for libvirt reviews, registering libvirt as a third review target alongside kernel and QEMU.
It follows the existing modular
src/target/layout (a TargetSpec implementation insrc/target/libvirt.rsover an embeddedresources/prompts/libvirt/corpus).boro reviewnow accepts--target libvirt, which selects the matching prompt corpus and reviewer persona, and warns when the selected target appears to mismatch the source tree.