style(utils): trim RunQemuImg godoc + move subprocess marker inline#41
Merged
Conversation
The 10-line godoc duplicated the "shell out because qemu-img is authoritative" rationale that lives inline on every other subprocess site in cocoonv2 (oci/erofs.go, cloudimg/inspect.go, ch/start.go, fc/start.go, hypervisor/utils.go). Trim the godoc to its actual contract (when to use, when NOT to use) and move the shell-out justification onto the exec.CommandContext call so the pattern matches sibling files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comment audit on 205 Go files turned up one trim candidate.
utils.RunQemuImg's 10-line godoc duplicated the "shell out because qemu-img is authoritative" rationale that all 5 other subprocess sites in cocoonv2 keep as a 1-line// shell out because…comment next to theexec.Commandcall:images/oci/erofs.go—// shell out because no Go EROFS writer library; mkfs.erofs is authoritative.images/cloudimg/inspect.go—// shell out because no Go inspector covers the full disk format matrix; qemu-img is authoritative.hypervisor/utils.go—// shell out because no Go ext4 formatter library; mkfs.ext4 is authoritative.hypervisor/cloudhypervisor/start.go—// shell out: the cloud-hypervisor binary is the authoritative VMM.hypervisor/firecracker/start.go—// shell out: the firecracker binary is the authoritative VMM.RunQemuImg's godoc was the only one carrying the rationale as a separate paragraph in the doc comment. Trim to:exec.CommandContextline for sibling parity.Audit summary (no other actionable findings)
Senior SKILL.md walk on 205 files:
/simplify x3 parallel (reuse/quality/efficiency, 27 findings total) — verified each, all bogus or known-design:
console/resize.go"goroutine never exits" —for range closed-channelis correct Go idiom;fc/start.go"subprocess leak" — relay is intentionally detached (Setpgid);cmd/vm/status.go"no ctx.Done() at outer loop" — ctx.Done IS the first outer case;console/relay.gochannel buffering — documented "caller closes conn" contract.cmd/vm/exec.go"byte-per-read alloc" — buffer allocated once outside loop, comment explains why bufio would over-read;images/oci/oci.go imageSizer"redundant stats" — each stat is a different file (blob/kernel/initrd);cloudimg/commit.go"double ValidFile" — pre-lock probe + in-txn race-safe check, different purposes.fc/snapshot.govsch/snapshot.gobuildSnapshotMeta— structurally different (FC trusts rec order, CH reconciles against config.json; FC mutates BootConfig, CH passes through).fc/api.go putJSONvsch/helper.go vmPutJSON— semantically opposite (FC retries idempotent PUTs, CH calls DoAPIOnce for non-idempotent). Unifying would manufacture abstractions that hide meaningful differences.make lintlinux+darwin: 0 issues.go test ./...: green.Test plan
make lintclean both GOOSesgo test ./...green// shell out…markers — pattern now uniform