Lab 2.4: Review It — Spot the Flaws in a PR

Background

Becoming a Firecracker contributor is half writing PRs and half reviewing them. A merge needs two maintainer approvals, which means the project runs on people who can read a diff and find what's wrong with it before it ships into something that runs thousands of untrusted guests per host. This lab puts you on the maintainer's side of the table: you are given a realistic Firecracker-style Rust diff with several planted problems, and you must find every one and write the review comments a maintainer would write.

This is a review-it lab. The diff is fictional but representative — a small change to a virtio / device-config path, exactly the kind of PR a Level-2-to-3 contributor opens. The flaws are the ones that recur on real Firecracker PRs: a missing test, an unwrap() that should propagate a Result, a missing CHANGELOG entry, an API back-compat break, a missing seccomp consideration, and a clippy violation. Find them all, then compare against the model review.

Why This Lab Matters for Contributors

• Reviewing teaches you to write better PRs: every flaw you learn to catch is one you stop shipping.
• Firecracker's review bar is specific and consistent. Knowing the five lenses — correctness, tests, compatibility, security/attack-surface, style — lets you both pass review and give it.
• Constructive review is a skill. A good comment is specific, cites the line and the principle, proposes a fix, and is kind. You practice that voice here.

Prerequisites

• Lab 2.3 complete — you have written a real fix and know what a good PR looks like from the inside.
• Lab 2.1 complete — you can read the vmm device/config code by role.
• The threat-model framing from the introduction: the guest is untrusted; every syscall and every byte of device code is attack surface.

The Five Review Lenses

Review like a maintainer means running the diff through five lenses, in order. Correctness first, because a correct-but-ugly change can be cleaned up, but a wrong change is wrong:

flowchart LR
    A["1. Correctness<br/>does it do the right thing? edge cases? panics?"]
    B["2. Tests<br/>is the new behavior pinned by a test?"]
    C["3. Compatibility<br/>API / snapshot / CHANGELOG impact?"]
    D["4. Security<br/>attack surface? seccomp? untrusted input?"]
    E["5. Style<br/>clippy, fmt, naming, comments"]
    A --> B --> C --> D --> E

The PR Under Review

Scenario: A contributor opens "Add max_queue_size to the virtio-block config" — a PR that lets users override the block device's virtqueue size in the drives config. Here is the full diff as it would appear in the GitHub "Files changed" tab. Read it with the five lenses before scrolling to the model review.

diff --git a/src/vmm/src/vmm_config/drive.rs b/src/vmm/src/vmm_config/drive.rs
--- a/src/vmm/src/vmm_config/drive.rs
+++ b/src/vmm/src/vmm_config/drive.rs
@@ -20,12 +20,15 @@ pub struct BlockDeviceConfig {
     /// Path of the drive.
     pub path_on_host: String,
     /// If set to true, it makes the current device the root block device.
     pub is_root_device: bool,
     /// If set to true, the drive is opened in read-only mode.
-    pub is_read_only: bool,
+    pub read_only: bool,
+    /// Maximum size of the device's virtqueue.
+    pub max_queue_size: u16,
 }

 impl BlockDeviceConfig {
     pub fn into_block(self, mem: &GuestMemoryMmap) -> Block {
-        let queue_size = DEFAULT_QUEUE_SIZE;
+        let queue_size = self.max_queue_size;
         let file = std::fs::File::open(&self.path_on_host).unwrap();
         Block::new(self.path_on_host, self.is_root_device, queue_size, file)
     }
diff --git a/src/vmm/src/devices/virtio/block/device.rs b/src/vmm/src/devices/virtio/block/device.rs
--- a/src/vmm/src/devices/virtio/block/device.rs
+++ b/src/vmm/src/devices/virtio/block/device.rs
@@ -55,6 +55,9 @@ impl Block {
     pub fn new(path: String, is_root: bool, queue_size: u16, file: File) -> Self {
+        // Allocate the virtqueue with the configured size.
+        let queue = Queue::new(queue_size);
+        let avail_ring = vec![0u8; queue_size as usize * 8];
         Block {
             path,
             is_root,
             queue,
+            avail_ring,
             file,
         }
     }

Now stop and write your review before reading on. List every problem you would raise, by lens, with the line it's on and the fix you'd suggest. Aim for at least six findings — there are at least that many.

The Model Review

Here is the review a maintainer would leave. Compare it against yours; each finding names the lens, the location, the principle, and a concrete fix.

Finding 1 — Correctness: unwrap() on a fallible file open (blocking)

#![allow(unused)]
fn main() {
let file = std::fs::File::open(&self.path_on_host).unwrap();
}

File::open returns Result; unwrap() turns any failure (missing path, permissions, the guest or user supplying a bad path) into a panic. In the VMM this is at best a crash and at worst a denial-of-service vector, since the path comes from user/orchestrator input. into_block must return a Result and propagate the error.

Review comment: "unwrap() here will panic on any open failure (bad path, EACCES). This path handles user-supplied input, so it must not panic. Please make into_block return Result<Block, BlockError> and use ?, mapping the io error to a typed error variant. See how the existing drive config surfaces open failures: rg -n 'File::open|fn into_block' src/vmm/src/vmm_config/."

Finding 2 — Compatibility: a renamed public API field (blocking)

-    pub is_read_only: bool,
+    pub read_only: bool,

is_read_only is a wire-API field (it appears in the JSON a client PUTs to /drives/{id}). Renaming it silently breaks every existing client and every saved config file — a back-compat break with no deprecation, no CHANGELOG note, and unrelated to the PR's stated purpose (adding max_queue_size). This is the single most serious issue in the diff after the panic.

Review comment: "is_read_only is part of the public API serialized into the /drives request body — renaming it breaks existing clients and config files, and it's unrelated to this PR. Please revert this rename. Confirm the serialized name with rg -n 'is_read_only|serde' src/vmm/src/vmm_config/drive.rs — API/snapshot back-compat is a hard line for us."

Finding 3 — Compatibility/Correctness: unvalidated, undefaulted new field (blocking)

max_queue_size: u16 is added with no validation and no default. virtio split-queue sizes must be a power of two within device limits; an arbitrary u16 (including 0, or a non-power-of-two, or something larger than the device supports) will either misbehave or crash. Because the field has no #[serde(default)], every existing client that doesn't send it now gets a deserialization error — another silent back-compat break.

Review comment: "max_queue_size needs (a) #[serde(default = ...)] so existing requests that omit it still parse, and (b) validation in the config validate() path: it must be a power of two and within the device's supported range, else reject pre-boot with a clear error. Right now 0 or a non-power-of-two reaches Queue::new. See the other fields' validate() in this file."

Finding 4 — Tests: no test at all (blocking)

The PR adds user-facing functionality (a new config field that changes device behavior) and adds zero tests. Firecracker requires integration tests for new functionality, and this change has obvious cases to pin: a valid max_queue_size is honored; an invalid one (0, non-power-of-two, too-large) is rejected; an omitted one uses the default.

Review comment: "This needs tests. At minimum: a unit test that validate() rejects 0 / non-power-of-two / over-max, and an integration test in tests/ that PUTs a drive with a valid and an invalid max_queue_size and asserts the accept/reject. New functionality requires integration coverage — see ls tests/integration_tests/functional/ | rg -i drive."

Finding 5 — Correctness: hand-rolled ring buffer duplicates virtqueue logic (blocking)

#![allow(unused)]
fn main() {
let avail_ring = vec![0u8; queue_size as usize * 8];
}

The device is hand-allocating an avail_ring Vec next to a Queue. Firecracker's virtqueue logic lives in the queue abstraction (and rust-vmm's virtio-queue); a device should not maintain its own parallel ring buffer. This is duplicated, untested machinery with an arithmetic bug waiting to happen (* 8 is a magic number, and queue_size as usize * 8 can be reasoned about only if queue_size is bounded — which Finding 3 shows it isn't, so this can also over-allocate). The Queue::new(queue_size) line is the right approach; the manual avail_ring should be deleted.

Review comment: "Drop the manual avail_ring — the queue abstraction owns ring memory; devices don't allocate it themselves. This duplicates virtqueue logic and the * 8 is an unbounded, magic allocation. Use Queue::new(queue_size) only. rg -n 'avail_ring|struct Queue' src/vmm/src/devices/virtio/ shows how the existing devices delegate this."

Finding 6 — Security: no seccomp consideration noted (discuss)

The PR doesn't introduce a new syscall visibly, but the reviewer must check: does the larger, user-controlled allocation or the new file-open path reach a syscall not in the block-device thread's seccomp allowlist? Any new syscall on a device path needs a resources/seccomp/ filter update, or it will be killed at runtime. The author should confirm.

Review comment: "Have you checked whether this changes the syscall surface of the block device thread (the larger allocation, the open path)? If it introduces any syscall not already allowed, resources/seccomp/<arch>.json needs updating, or the thread gets SIGSYS at runtime. Please confirm — rg -n 'block|vmm' resources/seccomp/ and the seccomp deep dive."

Finding 7 — Style/Process: missing CHANGELOG entry (blocking-lite)

A new user-facing config field is exactly what the CHANGELOG exists for, and there is no entry. CI's changelog check will be red.

Review comment: "Please add a CHANGELOG entry under the unreleased heading (Added): the new max_queue_size field on /drives. The changelog check is failing on this."

Finding 8 — Style: clippy and dead parameters (nit)

into_block takes mem: &GuestMemoryMmap but never uses it in the diff (if it's genuinely unused, clippy's unused_variables/-D warnings fails CI; if it's used elsewhere, fine). The doc-comment "Allocate the virtqueue with the configured size" describes what not why. Minor, but Firecracker runs -D warnings, so an actually-unused parameter blocks the merge.

Review comment: "If mem is unused after this change, clippy (-D warnings) will fail — drop it or prefix with _. And the // Allocate the virtqueue… comment restates the code; prefer a why comment or none."

Review summary the maintainer posts

Thanks for the PR! The max_queue_size feature is reasonable, but this needs another pass before it can go in. Blocking: the unwrap() panic on File::open, the unrelated is_read_only rename (please revert — it's an API break), the unvalidated/undefaulted new field, the missing tests, and the hand-rolled avail_ring. Please also add a CHANGELOG entry and confirm there's no new seccomp surface. Happy to re-review once these are addressed — the queue-size plumbing itself is on the right track. Requesting changes.

How to Verify Findings on a Real Checkout

A reviewer doesn't take the diff's word for what's an API field or how queues work — they check:

# Is is_read_only really serialized into the API? (Finding 2)
rg -n 'is_read_only|#\[serde' src/vmm/src/vmm_config/drive.rs

# How do existing config types validate and default fields? (Finding 3)
rg -n 'fn validate|serde\(default' src/vmm/src/vmm_config/

# Do devices hand-roll ring buffers, or delegate to a Queue? (Finding 5)
rg -n 'struct Queue|avail_ring|fn new' src/vmm/src/devices/virtio/

# What syscalls is the block path allowed? (Finding 6)
rg -n 'block|open|mmap' resources/seccomp/ | head

This is the same anti-staleness habit as every other lab: don't assert from memory, run the command.

Implementation Requirements

• A written review of the diff that independently found at least six of the eight findings — including the unwrap(), the is_read_only rename, and the missing tests.
• Each of your comments names the lens, the location, the principle, and a fix.
• At least one finding where you cite the rg command you'd run to verify it on a real checkout.
• A one-paragraph review summary that states what is blocking versus a nit, and ends with a clear verdict (request changes / approve).

Troubleshooting

I only found three or four problems

Re-run the five lenses explicitly, one at a time, instead of reading top to bottom. Most missed findings are the ones that aren't visible in the diff: the absent test, the absent CHANGELOG, the absent seccomp check, the absent default. Train yourself to look for what should be there and isn't.

I flagged something that isn't actually wrong

Good — false positives are part of review. The fix is to verify before you assert: run the rg command, read the surrounding code. A reviewer who blocks a PR on a wrong claim loses credibility fast.

My comments feel harsh

Re-read the model comments: each states the problem, the why, and a concrete fix, and several end with encouragement. "This is wrong" is not a review; "this panics on user input — return a Result and ?, like X does" is. Tone is covered in Community Interaction and PR Quality.

Expected Output

A review document structured by lens, e.g.:

CORRECTNESS
 - drive.rs into_block: File::open().unwrap() panics on user input -> return Result, use ?   [blocking]
 - block/device.rs: hand-rolled avail_ring duplicates queue logic; magic *8; unbounded       [blocking]
TESTS
 - no unit or integration test for the new field / validation                                 [blocking]
COMPATIBILITY
 - is_read_only -> read_only renames a public API field; unrelated; no deprecation             [blocking]
 - max_queue_size has no serde(default) -> breaks clients that omit it; no validation           [blocking]
 - no CHANGELOG entry                                                                          [blocking-lite]
SECURITY
 - unconfirmed seccomp impact of new alloc/open path                                            [discuss]
STYLE
 - possibly-unused `mem` param fails clippy -D warnings; restating comment                       [nit]

VERDICT: Request changes. Five blockers; feature direction is fine.

Stretch Goals

1. Review a real PR. Open an in-review Firecracker PR (gh pr list --repo firecracker-microvm/firecracker --state open) and draft a review through the five lenses. Compare with what the maintainers actually said. Do not post your draft.
2. Write the fixed diff. Rewrite the PR under review to address every finding: into_block -> Result, revert the rename, serde(default) + validate(), delete avail_ring, add the tests and CHANGELOG line.
3. Find a real unwrap() smell. rg -n '\.unwrap\(\)' src/vmm/src/vmm_config/ src/firecracker/src/ and judge each: which handle user input and should be Results? (Most existing ones are fine — learn to tell the difference.)
4. Map the back-compat surface. rg -n '#\[serde' src/vmm/src/vmm_config/ — list the field names that are part of the wire API. These are the names that must never silently change.

Validation / Self-check

You are done when you can answer these without notes:

1. Name the five review lenses, in order, and one Firecracker-specific question each asks.
2. Why is a guest- or user-triggerable unwrap()/panic! in the VMM a security issue, not just a style one?
3. Why is renaming is_read_only a blocking problem even though the code still compiles?
4. What two things does adding a new config field require for back-compat, and what breaks if you omit the serde(default)?
5. When does a change require a resources/seccomp/ update, and what happens at runtime if you forget?
6. What makes a review comment constructive rather than just correct?
7. How many of the eight findings are "absent thing" findings (something that should be present and isn't), and why are those the hardest to catch?

You have now walked the whole Level 2 workflow: navigated the repo, prepared a PR with Firecracker's exact practices, fixed a real good-first-issue with a test, and reviewed a flawed PR like a maintainer. You have the mechanics and the judgment to contribute surgical changes that two maintainers can approve without a five-round back-and-forth.

Next: Level 3 — Architecture and the Threading Model, where you stop fixing strings and start tracing a request from the API socket through VmmAction to a running microVM.