Libvirt review support#23
Conversation
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.
NathanChenNVIDIA
force-pushed
the
libvirt-review-support
branch
from
1d99b85 to
eb3d6f2
Compare
|
@nirmoy Thank you for the review, I will ping you separately when it is ready for a re-review |
NathanChenNVIDIA
force-pushed
the
libvirt-review-support
branch
from
eb3d6f2 to
de6eade
Compare
|
Hi @nirmoy , this is ready for a re-review. |
nirmoy
left a comment
nirmoy
left a comment
There was a problem hiding this comment.
Codex found this important issue during the follow-up review. I left one inline comment with fresh upstream libvirt validation output.
| - **Monitor calls**: only inside `qemuDomainObjEnterMonitor()` / | ||
| `...ExitMonitor()`. The domain lock is dropped during the call; re-check | ||
| `virDomainObjIsActive(vm)` afterwards before touching `vm->def`/private state. |
There was a problem hiding this comment.
Codex found this important issue: this QEMU-driver guide still gives a blanket rule that conflicts with the corrected locking guidance added in this same PR. It tells the reviewer to re-check virDomainObjIsActive(vm) after qemuDomainObjExitMonitor() before touching vm->def/private state, but resources/prompts/libvirt/subsystem/locking.md and resources/false-positive-digest-libvirt.md both correctly say not to demand that after every monitor exit when a domain job is held.
I validated this against a fresh upstream libvirt clone at 07b220964f968284533e589a2ac28cc465fd26ae. The live example is vCPU hotplug: the caller holds VIR_JOB_MODIFY, exits the monitor, then updates vm->def without an immediate active check:
qemu_driver.c:4395: if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
qemu_driver.c:4413: ret = qemuDomainSetVcpusInternal(driver, vm, def, persistentDef,
qemu_hotplug.c:6891: qemuDomainObjEnterMonitor(vm);
qemu_hotplug.c:6895: qemuDomainObjExitMonitor(vm);
qemu_hotplug.c:6903: vm->def->individualvcpus = true;
That is not a bug: the held domain job is the serialization mechanism that prevents concurrent destructive operations while the domain lock is dropped for the monitor call. As written, this prompt will make boro flag valid upstream libvirt hotplug paths as missing active checks. Please soften this to match the locking guide, e.g. only flag post-ExitMonitor stale-state use when the path holds no job, or when the specific cached value can actually change during the monitor call and unsafe reuse is proven.
There was a problem hiding this comment.
Thanks, I've updated the PR to address this, it is ready for a re-review.
NathanChenNVIDIA
force-pushed
the
libvirt-review-support
branch
2 times, most recently
from
0e999d3 to
4c02737
Compare
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, plus the resources/prompts/libvirt.local/ overlay directory. Symbol names and rules in the always-loaded guides are verified against the current upstream libvirt tree: domain jobs are virDomainObjBeginJob()/virDomainObjEndJob() with VIR_JOB_MODIFY; string parsing uses virStrToLong_ull, and virStrToLong_*/virStrToDouble reject trailing bytes only when the end pointer is NULL; virBuffer has no error state; and locking findings require a proven inverse lock edge rather than blanket mandates. Also add libvirt 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, build/portability, comments), written in libvirt terms with no kernel-only mandates. Make the findings validator domain-neutral so it is correct for every target. 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 personas), and wire it through ReviewTarget::Libvirt, the --target CLI value, and best-effort tree detection. Make the shared discovery/validation pipeline target-aware rather than kernel-only: one_shot_review(), false_positive_digest() and stage_instructions() are TargetSpec methods that default to the kernel corpus, and libvirt supplies its own. Thread the active target through build_reference_context(), load_false_positive_digest() and the specialist-stage loop, and add a test asserting the fully assembled libvirt payload carries no kernel-only/stale tokens. Gate the lore.kernel.org upstream-followup stage to ReviewTarget::Kernel:. lei requires a public-inbox index, but libvirt development is archived on HyperKitty/Mailman3 (no public-inbox endpoint), so a generic subject could otherwise pull unrelated kernel list traffic into a libvirt review. A libvirt-specific follow-up stage is documented as a follow-up. Signed-off-by: Nathan Chen <nathanc@nvidia.com>
NathanChenNVIDIA
force-pushed
the
libvirt-review-support
branch
from
4c02737 to
a94c5b3
Compare
arighi
left a comment
arighi
left a comment
There was a problem hiding this comment.
Thanks for working on this @NathanChenNVIDIA!
There's one small nit: in resources/review-validation-findings.md we still instruct a libvirt review to inspect Kbuild, EXPORT_SYMBOL(), and loadable module boundaries, which are kernel-specific concepts. We should probably generalize the shared prompt and move the kernel-specific guidance into a dedicated kernel prompt. But this can be addressed in a follow-up PR and improve in tree. For now, LGTM!
3 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.