Step 5: Implementation

You know the cause and the fix site. Now you write the smallest correct change that fixes it, in idiomatic Rust, that respects Firecracker's constraints — the minimal device model, the untrusted-guest threat model, the API and snapshot contracts — and that a maintainer can read in one sitting. The temptation at this step is to do more than the bug requires; resist it. A two-line fix with a one-line comment and a test beats a hundred-line refactor every time.

Goal

A minimal, correct, idiomatic fix on your feature branch, structured as one-logical-change commits, each DCO-signed, each passing the build and clippy. No scope creep, no drive-by refactors, no widening of the attack surface.

Minimal Diff Discipline

The fix should be as small as the root cause allows and no smaller. Before you write a line, decide the shape of the change from your Step 4 fix site:

• A validation gap → one check returning an existing error variant.
• A wrong value → one corrected expression, with a comment if non-obvious.
• A missing case → one added match arm or branch.
• An off-by-one / boundary → one corrected bound, plus a comment naming the invariant.

Then hold yourself to it. As you work, watch the diff stat and challenge anything unrelated to the bug:

git diff --stat origin/main   # are these the files the root cause predicted?
git diff origin/main          # is every changed line justifiable in one sentence?

If a file you didn't expect shows up, ask why. "While I was here I also tidied…" is exactly the scope creep reviewers flag — it inflates the diff, hides the real change, and risks an unrelated regression. Note the cleanup idea for a separate PR and move on. See PR quality.

Warning: Do not reformat surrounding code, rename variables for taste, or "modernize" idioms near your fix. tools/devtool fmt will reformat only what you changed; a diff full of whitespace churn is a fast path to "please rebase and drop the noise."

Write Idiomatic Rust the Codebase Will Accept

Match the conventions already in the file and crate, not your personal style. Firecracker has strong patterns; mirror them.

Error handling. Firecracker uses typed error enums (frequently with thiserror) and Result, not panics, on any path reachable from input. Find the error type for your subsystem and add to it rather than inventing a new mechanism:

rg -n "enum .*Error|#\[derive\(.*Error|thiserror" src/vmm/src/<subsystem>/

Return the right variant with a message an operator can act on. If the right variant doesn't exist, add one to the existing enum (a one-logical-change addition) rather than reaching for panic!/unwrap()/expect():

#![allow(unused)]
fn main() {
// Reject at the boundary with a typed, actionable error — not a panic deep in the builder.
if cfg.vcpu_count == 0 {
    return Err(MachineConfigError::InvalidVcpuCount); // add variant to the existing enum
}
}

Note: unwrap()/expect()/panic! on a guest- or operator-reachable path will draw review fire and may trip clippy. They are acceptable only for true invariants that cannot be violated by input — and even then, prefer expect("reason the invariant holds") so the reason is in the code. On the untrusted-guest data plane (virtio), a panic is a denial-of-service vector; bounds-check guest-supplied descriptor fields and return an error, never assume.

Respect the constraints from Step 4. Re-read your four-dimension analysis and let it shape the code:

Idioms to match. Use the iterator/?/pattern-matching style the surrounding code uses; prefer if let/match over manual unwrapping; keep functions small; put the comment where the why isn't obvious, not where the what is. When in doubt, find two nearby examples of the same kind of change and imitate them.

Add a Comment Only Where It Earns Its Place

A non-obvious fix gets one comment that explains why, and cites the issue so the next reader understands the constraint:

#![allow(unused)]
fn main() {
// vcpu_count must be >= 1; the builder assumes it when creating vCPUs (see #NNNN).
}

Do not narrate the obvious. // add one over x += 1 is noise; the invariant comment above is signal because it encodes knowledge that isn't in the code.

Commit in One-Logical-Change Units, Signed Off

Firecracker wants each commit to be a single logical change that passes the build and tests on its own, with a DCO sign-off. This is not bureaucracy — it is what makes the PR reviewable commit-by-commit and bisectable later.

# Stage only the production change for the fix commit (tests come in their own commit in Step 6).
git add src/vmm/src/resources.rs
git commit -s -m "Reject vcpu_count of 0 in machine config validation

The machine-config handler accepted vcpu_count == 0, which the builder
assumes to be >= 1, producing a failure at InstanceStart instead of a
clear 400 at configuration time. Validate the lower bound at the API
boundary and return MachineConfigError::InvalidVcpuCount.

Signed-off-by: Your Name <you@example.com>"

The mechanics:

• -s appends the Signed-off-by: line; the DCO bot matches it to your commit author email. Get this right or the PR check fails. Amend with git commit --amend -s if you forget. See licensing-and-dco and Level 2.
• Title ≤ 72 chars, imperative mood ("Reject…", not "Rejected…" or "Fixes…").
• Body explains the why — the symptom, the assumption that was violated, the fix. The reviewer reads this before the diff.
• One logical change per commit. If your fix legitimately needs two independent changes (e.g. add an error variant, then use it), two commits is fine. A "fix + unrelated cleanup" in one commit is not.

Tip: Keep the production fix and its tests as separate commits (or at least reviewable units). It lets a reviewer see the fix, then see the test that proves it — and lets you demonstrate, in Step 6, that the test is red without the fix commit applied.

Keep It Building and Clippy-Clean as You Go

Do not save all the gate-passing for Step 7. Build and lint after each change so problems surface while the context is fresh:

tools/devtool build                 # compiles? (musl by default)
tools/devtool checkstyle            # fmt + clippy + sorting checks
# Clippy is warnings-as-errors in CI:
#   cargo clippy --all --all-targets --all-features -- -D warnings

A clippy warning on your new code is a free code review — it often points at exactly the non-idiomatic construct a maintainer would flag. Fix it now, not after you've moved on.

Deliverable for Step 5

• A minimal fix on your feature branch, touching only the files the root cause predicted.
• Every changed line justifiable in one sentence (git diff origin/main is tight and on-topic).
• Typed error handling on input-reachable paths — no new unwrap/panic on guest- or operator-reachable code.
• One-logical-change commit(s), each DCO-signed with a -s sign-off and a why-focused body.
• tools/devtool build and checkstyle green on the branch.
• No new emulated device, syscall, snapshot-format change, or attack surface unless that is the issue.

Rubric Hooks

This is the Fix quality dimension (22 pts), the heaviest in the rubric: minimal diff, idiomatic Rust, typed errors, no scope creep, attack-surface and compatibility discipline, clean one-logical-change commits. A tight, on-topic fix with a justified comment scores high; a broad fix with drive-by refactors, a symptom-site patch, or new unwraps on the data plane scores low. Commit hygiene and DCO also feed PR craft. See the evaluation rubric.

Validation / Self-check

Before advancing to Step 6:

1. Every changed line is explained by your root cause; nothing is "while I was here."
2. You return a typed error (not a panic) on any input-reachable path you touched.
3. You did not add a device, a syscall, a snapshot field, or guest-reachable surface that the issue didn't call for.
4. Your commits are one-logical-change, DCO-signed, with imperative ≤72-char titles and why-focused bodies.
5. tools/devtool build and checkstyle pass on the branch.
6. You can explain, in one sentence each, why this fix site (not the symptom site) and why this shape (not a larger one).

Then go to Step 6: Testing.