Libvirt review support

resources/false-positive-digest-libvirt.md

@@ -0,0 +1,120 @@
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
+# What NOT to flag — false-positive digest (libvirt specialist stages)
+
+This is a tight subset of `false-positive-guide.md` distilled for the specialist
+stages. Apply these rules BEFORE you emit a concern, not after. If a concern
+fails any applicable rule, drop it. The full guide still runs at consolidation;
+your job here is to stop weak concerns at the source.
+
+## Core principle
+
+If you cannot point to **specific code in the diff** that proves the issue, do
+not emit it. "Could happen," "might race," "should validate" → drop.
+
+## Concrete rules
+
+### 1. Defensive programming requests
+Do NOT request bounds checks, NULL checks, or input validation unless you can
+show:
+- the value comes from an untrusted source (XDR-decoded RPC argument,
+  client-supplied XML, guest-agent/QMP reply, file/`/proc` content), AND
+- an actual code path in the diff reaches the use without intervening
+  validation.
+
+`g_new0`/`g_strdup`/`g_strdup_printf` abort on OOM — do NOT ask for a NULL check
+after them. Generic "add a check for safety" with no proof: drop.
+
+### 2. API misuse assumptions
+Do NOT report "caller might not hold the lock/job" or "caller might pass NULL"
+without showing the actual calling path that does so. libvirt functions often
+document preconditions (a locked+ref'd `virDomainObj`, a held job) that callers
+must satisfy — check the callers before flagging.
+
+### 3. Unverifiable claims from the commit message
+The commit message is not evidence on its own. If the author says "this is safe
+because Y," verify Y from the code. Do not flag based only on a missing
+justification, and do not exonerate based only on the changelog's promises.
+
+### 4. Locking complaints — caller-first
+Before emitting any "missing lock" concern, trace 2-3 levels up the call chain.
+If a documented caller already holds the object lock or the domain job, drop the
+concern. Taking `driver->lock` (e.g. via `virQEMUDriverGetConfig()`) while a
+domain is locked is a normal, short critical section — not an ordering bug by
+itself. A lock-order concern needs two concrete paths taking the same pair of
+locks in opposite order.
+
+### 5. Monitor / job state after ExitMonitor
+Do NOT demand a `virDomainObjIsActive(vm)` re-check after every
+`qemuDomainObjExitMonitor()`. The held domain job blocks concurrent destructive
+operations, so updating `vm->def`/private state right after ExitMonitor is
+normal. Flag stale-state use only when the path holds no job (or the specific
+value could actually change during the monitor call) AND reuse is unsafe.
+
+### 6. Use-after-free vs. use-then-free
+Only flag the sequence `alloc → use → free → use` (or `alloc → free → use`).
+`alloc → use → free` (normal cleanup) is not a UAF. If ownership transferred
+(added to a hash/list, handed to a `virThreadPool` job, an event callback, or a
+timer that took a ref), the original holder unref'ing is expected.
+
+### 7. Resource leaks — ownership matters
+Not a leak when:
+- the object was added to a hash/list/`virDomainObjList` for later cleanup,
+- ownership transferred to another subsystem or an async callback,
+- a `g_autoptr`/`g_autofree`/`g_auto(virBuffer)` variable frees it on scope exit,
+- `g_steal_pointer()` handed it off deliberately.
+Check for an error label / `g_auto*` / `virObjectUnref`/`virDomainObjEndAPI` in
+the diff before flagging.
+
+### 8. Races — show two concurrent paths
+A race concern must name both paths and the contested state. "X could race with
+Y" with no specific call sites is not a finding. Code that detects the invalid
+state and aborts is only safe if every instruction between the race window
+opening and the abort point is safe under the invalidated state.
+
+### 9. Uninitialized variables
+Only flag **reads** of uninitialized memory. Assigning a value initializes it.
+Passing an uninitialized variable to a function that writes it before reading (a
+common out-parameter pattern, e.g. `virStrToLong_*`) is fine.
+
+### 10. NULL dereference
+Before flagging a NULL deref, check whether an earlier line (or the calling
+convention) guarantees non-NULL. A `g_new0` result, a `virDomainObj` returned
+locked+ref'd by `virDomainObjListFindByUUID`, or a value already checked with
+`if (!p) ...` does not need another NULL check downstream.
+
+### 11. Style and naming
+Do not emit style complaints (naming, function size, comment style) as
+substantive findings. They belong in commit-message concerns (`msg:style`) at
+most, and usually not at all.
+
+### 12. Patch-series context
+If the diff is patch N of a series and a concern is fixed in a later patch of
+the same series, treat it as not-a-finding (or note the later-fix patch).
+
+### 13. Fixed old-code bugs
+Do NOT emit a concern merely because the removed/old code had a real bug. If the
+reviewed diff moves, deletes, initializes, checks, or otherwise fixes that old
+behavior, the old bug is evidence the patch is a fix, not a finding. Only report
+it if the new/right-side code still has the bug, the fix is incomplete, or the
+patch introduces a different bug.
+
+### 14. Function-like macro expansion
+Do NOT reason from the unexpanded spelling of a function-like macro body (e.g. a
+`VIR_*` or glib `g_*`/`G_*` macro). For any concern that depends on macro
+semantics, expand the complete invocation chain token by token: bind formal
+parameters to actual arguments, substitute every matching preprocessing token,
+and rescan for nested expansions. Punctuation or member-access operators do not
+make a matching parameter token literal. Account for stringification, token
+pasting, and variadic arguments when present. Keep the concern only if it
+remains true in the final expanded token stream.
+
+## How to write a concern that survives consolidation
+
+- Name the function, file, and line/region from the diff.
+- Quote or paraphrase the specific code, not the general pattern.
+- State the trigger condition that actually fires (not "if input is malformed").
+- For locking/UAF/race, list the call chain or sequence you traced.
+
+If you cannot do these for a concern, **do not emit it**. The consolidator will
+not invent the evidence; it can only drop what you wrote.

resources/one-shot-review-libvirt.md

@@ -0,0 +1,69 @@
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
+# Single-pass review (boro, libvirt)
+
+You are performing **one consolidated pass** over a libvirt patch that must
+cover the same dimensions as the multi-stage protocol, without running separate
+model calls per stage. libvirt is a privileged C management daemon (built on
+GLib) that XDR-decodes client RPC, parses domain/network/storage/nwfilter XML,
+and drives hypervisors (QEMU/KVM, LXC, ...).
+
+1. **Intent / architecture** — public API/XML/RPC design, maintainability,
+   conceptual flaws.
+2. **Commit message** — English spelling, grammar, syntax, and clarity (subject
+   and body); misleading or incomplete changelog vs the diff; missing updates,
+   API/struct/callback completeness, semantic correctness.
+3. **Execution flow and validation provenance** — branches, error paths (`goto
+   cleanup;`), return-value checks, off-by-one. Track the identity of every
+   validated candidate through its final use or return. When code substitutes a
+   sibling, cached value, or second lookup result, prove the replacement
+   satisfies every predicate checked on the original object; set/list membership
+   alone does not carry per-object properties.
+4. **Resources** — leaks, UAF, `virObject` refcount balance, `virDomainObjEndAPI`
+   pairing, fd/`virCommand` cleanup, and event-loop/thread-pool/timer teardown
+   symmetry.
+5. **Locking / concurrency** — object-lock coverage (`virObjectLock` /
+   `virObjectRWLock*`), lock-order inversions (proven by two opposite-order
+   paths, not a lone edge), domain-job usage (`virDomainObjBeginJob` /
+   `virDomainObjEndJob`), monitor enter/exit state handling, and blocking work on
+   the event-loop thread.
+6. **Security** — bounds, integer overflow, TOCTOU on paths in a root daemon,
+   and information disclosure to a less-privileged client. Treat XDR-decoded RPC
+   arguments, client-supplied XML, and guest-agent/QMP replies as untrusted.
+   A new public RPC API missing an ACL/polkit check is a real finding.
+7. **Build / portability** — for every newly referenced symbol, check that the
+   caller's and provider's build conditions match. libvirt gates code with
+   `WITH_*` macros (from the meson-generated `config.h`), per-driver/per-platform
+   conditionals, and symbol-version files (`*.syms`). Prove that whenever the
+   caller is compiled the provider exists; a new public symbol must be added to
+   the matching `.syms`, and a new wire element must bump the RPC protocol. Use
+   the checked-out tree as authoritative.
+8. **XML / RPC / driver surface** — parse↔format round-trips for `vir*Def`,
+   RNG-schema and protocol/`.x` consistency, and correct escaping of generated
+   XML/commands. Never assume client input matches the schema.
+
+Every finding must carry concrete proof appropriate to its issue type: the
+relevant code or text facts, a reachable trigger or witness when applicable, the
+violated invariant or direct contradiction, and the concrete failure or
+user-visible defect. Use a witness state and path for execution flow, an
+interleaving or lock-order cycle for concurrency, an acquisition/handoff/cleanup
+path for resources, an attacker-controlled input path for security, and exact
+contradictory text for comment or commit-message issues. Do not use "may",
+"might", "could", or "not guaranteed" as a substitute for missing evidence.
+
+Every build/portability finding must state a concrete proof: the exact condition
+under which the caller is compiled, the exact condition guarding the
+declaration/definition/export in the checked-out tree, and the resulting
+compile, link, or runtime `failure`. Verify these with repository tools.
+
+Be skeptical of the commit message. Prefer reporting a suspected issue with
+clear reasoning over silence.
+
+Do not report the bug that the patch is fixing. A defect visible only in
+removed/old code is not a finding when the new/right-side diff removes,
+reorders, initializes, checks, or otherwise fixes it. Report only if the new
+code still has the defect, the fix is incomplete, or the patch introduces a
+different bug.
+
+When the diff is documentation-only or trivial comment fixes, return an empty
+`findings` array.

resources/prompts/libvirt/callstack.md

@@ -0,0 +1,53 @@
+# Execution-Flow and Call-Stack Verification (Libvirt)
+
+When you suspect a bug, **prove the reachable path** before reporting it. A
+finding without a concrete call chain is usually a false positive.
+
+## Build the chain explicitly
+
+State the path from a caller- or attacker-reachable entry point to the defect,
+naming each function:
+
+```
+remoteDispatchDomainSetMemory()      <- client RPC, untrusted args
+  -> virDomainSetMemory()            <- public API + ACL check
+    -> qemuDomainSetMemoryFlags()    <- driver, takes domain job
+      -> qemuDomainObjEnterMonitor() <- drops domain lock
+        -> qemuMonitorSetBalloon(newmem) <- newmem not bounded
+```
+
+## Libvirt entry points worth tracing back to
+
+- **RPC dispatch** → `remoteDispatch*` / `*Helper` (generated), then the
+  `vir*` public API, then the driver method in the `virHypervisorDriver`
+  table.
+- **XML parsing** → `virDomain*DefParse*`, `virNetworkDefParse*`,
+  `virStorageVolDefParse*`, `virXPath*` accessors.
+- **Guest-agent / monitor input** → `qemuAgentCommand` /
+  `qemuMonitorJSON*` reply handlers; treat the parsed JSON as untrusted.
+- **Event loop callbacks** → `virEventAdd*` handles/timeouts, RPC keepalive.
+- **Thread-pool jobs** → `virThreadPool` worker functions.
+
+## Context questions to answer
+
+- **Which locks are held, in what order?** Is the driver state lock held while
+  taking a per-domain lock (or vice versa)? Crossing the established order is a
+  deadlock. Is a domain job (`virDomainObjBeginJob`) held where required?
+- **Is a lock dropped mid-sequence?** Around `EnterMonitor`/`ExitMonitor` the
+  domain lock is released. A held domain job blocks concurrent destructive
+  operations, so reusing `vm->def`/private state right after `ExitMonitor` is
+  normally fine — only a concern when the path holds no job, or the specific
+  state could actually change during the call and its reuse is unsafe (see
+  locking.md).
+- **Who owns the object?** Did this path take a ref (`virObjectRef`) it must
+  release, and does `virDomainObjEndAPI` run on every exit?
+- **Can a client trigger this concurrently** from two connections, racing on the
+  same domain/object?
+
+## Confirm, don't assume
+
+- If the bound/NULL check is in the caller (or the generated ACL/arg-validation
+  layer), the "missing check" is not a bug — show the caller.
+- If a field is set and read under the same lock, there is no race.
+- If an error path `goto`s cleanup or relies on `g_auto*`, read to the end of
+  the function before claiming a leak or UAF.

resources/prompts/libvirt/coding-style.md

@@ -0,0 +1,80 @@
+# Libvirt Coding Style
+
+Derived from libvirt's `docs/coding-style.rst` and `docs/hacking`. Libvirt has
+its own conventions enforced by `cppcheck`, `syntax-check` (the `make
+syntax-check` rules under `build-aux/`), and `clang-format` for some files. Flag
+deviations as **Low** severity (style/cosmetic) unless a style issue also causes
+a real bug (then use the bug's severity). Be specific and quote the offending
+line; do not bikeshed.
+
+## Whitespace and layout
+
+- Indent with **4 spaces, never tabs**. No trailing whitespace. Files end with a
+  single newline.
+- Lines should stay within ~80 columns.
+- One statement per line; one declaration per line.
+
+## Braces
+
+- Braces are mandatory on multi-line bodies. For a **single-statement** body,
+  libvirt omits the braces (`if (cond)\n    return -1;`) — the opposite of
+  QEMU. Don't add braces around a one-line body, and don't drop them when any
+  branch of the same `if/else` needs them.
+- Opening brace of a control block goes on the **same line**; opening brace of a
+  function definition goes on **its own line**.
+
+## Naming
+
+- Functions and types use a subsystem prefix in `lowerCamelCase` /
+  `UpperCamelCase`: `virDomainObjListFindByUUID`, `qemuProcessStart`,
+  `virStorageVolDef`. Public symbols are `vir...`; per-driver code uses its
+  driver prefix (`qemu`, `lxc`, `virNetwork...`).
+- Enums: `VIR_DOMAIN_FOO_BAR`; macros: `UPPER_CASE`.
+- Typedefs are `CamelCase`, with a matching pointer typedef
+  (`typedef struct _virX virX; typedef virX *virXPtr;` — newer code often uses
+  `virX *` directly rather than the `Ptr` alias).
+
+## Memory and strings
+
+- Use GLib allocators: `g_new0`, `g_strdup`, `g_strdup_printf`, `g_free`. Use
+  `g_autofree`, `g_autoptr()`, and `g_auto(virBuffer)` for scope-based cleanup;
+  this is strongly preferred over manual `cleanup:` goto labels in new code.
+- `g_new0`/`g_strdup` abort on OOM — do **not** add a NULL check after them.
+- `VIR_ALLOC`/`VIR_STRDUP` have been removed; use `g_new0`/`g_strdup`. `VIR_FREE`
+  still exists (it zeroes the pointer, unlike bare `g_free`), but new code prefers
+  `g_autofree`/`g_clear_pointer`.
+- Build strings with `virBuffer`, escaping with `virBufferEscapeString` /
+  `virBufferEscapeShell`; don't hand-roll XML/shell escaping.
+
+## Control flow and cleanup
+
+- New code prefers `g_auto*` cleanup and early `return` over the historical
+  `goto cleanup;` / `goto error;` pattern, but matching the surrounding file's
+  existing style is acceptable.
+- `ignore_value()` wraps a deliberately-unchecked return; a silently-dropped
+  return value without it is flagged by syntax-check.
+
+## Strings, printf, and gettext
+
+- User-facing messages are wrapped in `_()` for translation; format strings must
+  be string literals (syntax-check enforces this for `virReportError`).
+- Use `%1$s`-style positional args in translated format strings (a libvirt
+  syntax-check rule); a bare `%s` in a translated message is flagged.
+
+## Includes / headers
+
+- `<config.h>` is included first (via the build system); then system headers,
+  then libvirt headers. `internal.h` provides the common macros.
+
+## Comments
+
+- `/* ... */` block comments. Keep them about intent, not narration.
+
+## What to flag (and how)
+
+- Quote the line and name the rule: "tab indentation; libvirt uses 4 spaces",
+  "reintroducing the removed `VIR_ALLOC`; use `g_new0`", "format string not a literal",
+  "missing `_()` on a user-facing message", "`%s` instead of positional
+  `%1$s` in a translated string".
+- Keep style points Low and brief. Lead with substantive correctness/security
+  findings; don't pad the report with nits.

resources/prompts/libvirt/false-positive-guide.md

@@ -0,0 +1,49 @@
+# Avoiding False Positives (Libvirt)
+
+Most rejected review comments are false positives. Before keeping a finding,
+clear it against this guide. When in doubt and you cannot prove the path,
+**drop it** — a wrong comment costs maintainer trust.
+
+## Read enough context first
+
+- Use `read_files` / `git_show` / `git_blame` to read the **whole** function and
+  its callers, not just the diff hunk. Many "missing checks" or "missing
+  unlocks" are handled one frame up or by a `g_auto*`/`virDomainObjEndAPI`
+  cleanup you can't see in the hunk.
+- The diff shows a delta. The bug must be real in the *resulting* code.
+
+## Libvirt-specific non-bugs (do NOT report these)
+
+- **`g_new0` / `g_strdup` returning NULL**: they `abort()` on OOM by design. A
+  missing NULL check after them is not a bug. (The `g_try_*` allocators *can*
+  return NULL and must be checked — know which allocator is in use.)
+- **`g_autofree` / `g_autoptr` / `g_auto(virBuffer)`**: these free automatically
+  at scope exit. A "leak" on an early return where the owner is an autoptr is
+  not a leak. Conversely, a value that is `g_steal_pointer`'d out is
+  intentionally not freed here.
+- **`virDomainObjEndAPI()`**: this both unlocks and unrefs. Code that looks up a
+  domain and calls `virDomainObjEndAPI(&vm)` at the end is correctly balanced;
+  don't claim a missing unlock or unref.
+- **Lock dropped around a monitor call**: `qemuDomainObjEnterMonitor` /
+  `...ExitMonitor` intentionally release the domain lock during the QMP round
+  trip; that is the design, not a race — only flag it if state read *after*
+  ExitMonitor assumes nothing changed and that assumption is actually unsafe.
+- **`ignore_value(...)`**: a deliberately-unchecked return; not a bug.
+- **Error returned as -1 with `virReportError` already called**: that is the
+  contract. Don't ask for an error message that's set two lines up.
+- **Driver code behind a capability/version check**: feature-gated paths
+  (`virQEMUCapsGet`, version checks) are intentional compatibility handling.
+
+## Reportable vs not
+
+- Reportable: a concrete, reachable path where client RPC / XML / guest-agent /
+  QMP input causes OOB, UAF, leak, NULL deref, deadlock, an ACL bypass, or wrong
+  behavior — with the chain shown.
+- Not reportable: "could be cleaner", "might want a check" with no demonstrated
+  trigger, or speculation about callers you didn't read.
+
+## Calibrate to the changelog
+
+If the commit message explains a trade-off or says a follow-up handles X, factor
+that in. Don't report something the author documented as intentional — unless
+the code contradicts the message (that mismatch *is* reportable).