Repository navigation
review: whole-repo static audit, 16 fixes, cuts, docs against the code - #312
Merged
Merged
Conversation
The json engine test sent on fsnotify's own Errors channel, which kqueue closes behind a pipe the race detector cannot see, so go test -race failed on darwin. The branch belongs to Broadcaster; test it there and drop the json notifier's test-only watcher field.
…ding eject vm.remove-device drops the device from the VM config at once but keeps it in device_tree until the guest ejects. A vm net retry after an eject timeout saw no live NIC, deleted the TAP and truncated the record without asking the guest again; fs and device detach retries failed with "not attached". LiveNICs now lists pending NIC ejects unplaced so reconcile re-requests and waits, and fs/device detach find the id in device_tree as disk detach does.
A vm net add killed between the device PUT and the record append left FC with an unrecorded NIC that a snapshot then captured bound to the source TAP name, so every clone of it failed or reused the source TAP. Cloud Hypervisor refuses this since #302; the FC --pci pause check now does too.
FinalizeCreate commits the record durably while cidata.img sat in the page cache. A host reboot in that window left an empty or zeroed cidata the next start still attached, so cloud-init silently got no user or network config and nothing regenerates it. Cost: one fsync of a 1 MiB file per cloudimg create or clone.
The truncation check compared the first 64 bytes for equality, so a log shorter than that at open looked truncated on its next append and was copied again from offset 0. A head that still starts with the old one is growth, not truncation.
Request contexts did not derive from the daemon context, so Shutdown waited the full 2 s timeout whenever a /v1/events subscriber (sandboxd always is) was connected, and the stream handler outlived it.
f.Fd() switched the CH PTY to blocking mode, so closing it on SIGINT or SIGTERM no longer woke the pending read: the command hung until the guest printed and left the terminal raw. Take the fd through SyscallConn instead.
The help printed (default 432), the decimal form of 0660, so a user copying the style typed 660 and got mode 0o1224. The flag now defaults to 0, which the daemon already maps to 0660, and the help names the octal form.
Every other streaming verb closes its source when the context is canceled; import did not, so SIGINT or SIGTERM waited out the whole extraction.
…r one fails With refs owned by both backends, a failure in the second returned before the log loop, so refs the first had already removed were never reported.
The header went straight to stdout, outside the tabwriter that pads the rows, so its columns never lined up.
The rollback ran CNI DEL on every NIC it tried, including recorded NICs a start or restore had just rebuilt with their saved IP. That released the IP in IPAM while the records kept it, so a create elsewhere could take it and every later start failed with "requested IP not available". A recorded NIC now only loses its TAP; the next start re-plumbs it.
Firecracker's stderr went to /dev/null, so an error it prints before its logger starts (an argument it rejects, a bad config) left only the API socket timeout and an empty vm logs. Cloud Hypervisor already writes stderr to the process log.
Cobra adds the same completion command when none is defined, with bash completion V2 and --no-descriptions; cocoon completion <shell> keeps working as docs/install.md shows.
- restore marker: start clears a stale marker since #311, so the FinalizeRestore warning, the restoreDirtyName comment and the apply quarantine comments described a refusal that no longer happens - create, gc, netresize, localfile Delete, types.VM runtime and placement comments rewritten to what the code does - disk name error names the reserved cocoon- prefix it actually rejects - drop design-doc section refs (§N) that point outside the repo, and comments that restate the code
Index entries come only from materialize (&e, never nil, a JSON null decodes to a zero entry) and the two backends' commits, and removal uses delete(); the nil checks date from the whole-file index.
Every caller passes a non-nil VM: the embedded record VM, or a FindVM result that is non-nil on success.
Every caller passes a loaded config: cmd/core here and cocoon-macos's cni.New (built from netConf, never nil).
Both callers hand ToVM a record meta decoded fresh for this call; the clone dates from the in-memory index.
Since #306 Prepare always places into a freshly made scope, whose cpuset.cpus is empty, so the already-placed shortcut never fired and cost a sysfs read on every pinned launch.
Recovery reads only Payload.Mode and Payload.Cleanup; Kind was written on every lease and never consulted. Old rows that carry it still decode.
PRAGMA data_version waits at most busy_timeout and Conn.Close already waits for an in-flight check, so the cancel plumbing guarded nothing and repeated cancel() in four cleanup tails.
A uint64 marshals to its decimal digits; strconv.AppendUint writes the same bytes with no error branch.
- vm.md: a failed start stays error until the next successful start; a restore that fails before it stops the VM leaves no marker - networking.md: start and restore finish an interrupted vm rm instead of rebuilding NICs; FC guest hints print after every resize - daemon.md: a leased delete tombstone rolls back; SSE events are named change with the kind in type - cli.md: restore admits error VMs, --dns takes semicolons, --config, reseed skips, placement cleared only on stop, one FC console client - devices.md: fs tag and device id patterns; FC hints print on every call - gc.md, known-issues.md, snapshots.md, index.md: cni orphan records, a killed snapshot save, eject re-send on every guest, hibernate snapshot lifetime, SIGWINCH only on the CH PTY console - install.md: v0.7.7 release URLs - vm run --queue-size and vm debug --balloon help name CH-only and auto
…branches - build, lint and test workflows declare contents: read like the release and os-image workflows already do - build-os-images: no os-image/windows dir and no .skip-generic-build marker exist any more - meta-upgrade.py reads and writes the json as UTF-8, matching Go - vm-inspect.sh: grep -c already prints 0, so || echo 0 printed a second line
- known-issues: an FC clone sends from the source's guest MAC through the unchanged TC redirect, so the bridge table flaps; cloud-hypervisor#7356 closed unfixed - daemon.md: dead-VMM convergence applies to running records or an open interval; the 0750 run dir is the daemon's only when it creates it - install.md: the sqlite engine refuses NFS, CIFS/SMB and FUSE roots
… too Only the sqlite branch checked for the conversion manifest, so a json root whose json-to-sqlite convert had saved its manifest but not yet created meta.db got its namespace files rewritten under the manifest's pinned digests, and the resumed convert then failed as a changed source.
…eeded collectBlob printed "collected" before returning remover errors, so an EACCES or EBUSY left both a success line and a failure for the same blob.
A capture killed after its last write leaves a gap in round numbers whose files the next round does not list; migrate capture pauses like save and hibernate.
`return id, b.view(ctx, fn)` where fn sets id relies on id being read after the call; the spec leaves the order between a plain operand and a call in the same return unspecified. gc happens to read it after, so nothing fails today. Take the error first in the seven read paths that did this.
… once func(pid int) error appeared in three signatures and func(*VMRecord) error in three; /code rule 8 gives a repeated func type a name.
Layout rule: method-less vocabulary types cluster ahead of the first method-bearing type in the file.
… vsock Clone and restore results are built by SetRunningSockets after resume; the extra Inspect repeated the same vsock socket existence check and cost a meta read and a process check per clone or restore of a VM without vsock.
The non-root runcmd interpolates the password raw into sh -c; the comment named only the YAML scalar.
refuseUnrecordedDevices now also refuses an unrecorded NIC, and the store's data dirs come from utils.EnsureDirs, not MkdirAll.
…a wait Review of the previous commit found two regressions: - device detach accepted any id present in device_tree, so --id _disk0 or a NIC id hot-removed that device instead of failing as not attached; VFIO detach goes back to Config.Devices - listing pending NIC ejects as orphans made every vm net on a Windows clone with a stranded hot-swap eject wait 30 s and fail vm net now re-sends vm.remove-device once for each pending NIC eject and goes on, so a retry after an eject timeout still asks the guest again. docs/daemon.md: a delete tombstone rolls back only while leased.
CMGS
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| _ = f.Close() | ||
| logFile, createErr := os.OpenFile(fcLog, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o600) //nolint:gosec | ||
| if createErr == nil { | ||
| defer logFile.Close() //nolint:errcheck |
The quarantine, interrupted-restore and creating tests passed on any error, and the delete-close test counted entries without checking their kinds.
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.
Static whole-repo audit after #311: no hardware run in this round. Every production Go file was read in full by 13 package readers, 13 flow readers walked the crash windows of each cross-file flow (6 flows are new: hibernate-wake, image-pull-import, device-hotplug, daemon-api-console, gc-orchestration, create-run), 2 docs readers checked ~900 doc claims against the code, and one infra reader read every non-Go file. An adversarial verifier and a Codex review re-read the final diff; two regressions they found in an earlier version of the CH eject fix are corrected here, and one cut was withdrawn.
Prod Go +177/-250 (net -73), test +130/-92, Go comment lines +54/-64 (every added line rewrites an existing comment).
Fixes
vm netretry after an eject timeout asks the guest again (fa85a08,009b60a).vm.remove-devicedrops a device from the VM config at once and keeps it indevice_treeuntil the guest ejects, so the retry saw no live NIC and reclaimed the host side without re-requesting the eject (Windows drops vetoed ejects).vm netnow re-sends the request once per pending NIC eject and goes on.vm fs detachfinds a pending fs device indevice_treethe way disk detach does; VFIO detach keeps matching onlyConfig.Devices, so an arbitrarydevice_treeid (_disk0) is still refused.464efd6). Avm netadd killed between the device PUT and the record append left an unrecorded NIC that a snapshot captured bound to the source TAP name, so every clone of it failed or reused the source TAP. CH has refused this since review: 5 fixes, docs from hardware, and tests that prove what they name #302.cidata.imgis fsynced before the create commits its record (43cec2b). A host reboot in the writeback window left an empty cidata that every later start attached; nothing regenerates it. Cost: one fsync of a 1 MiB file per cloudimg create or clone.973cf73). The rollback ran CNI DEL on recorded NICs a start or restore had just rebuilt, releasing their saved IPs in IPAM while the records kept them. Regression test included./v1/eventsstreams at once (58fc94c). Request contexts did not derive from the daemon context, so every stop with sandboxd subscribed waited the full 2 s. Regression test included.vm consoleexits on signal with a CH direct-boot PTY (df65e1f).f.Fd()switched the PTY to blocking mode, so closing it no longer woke the pending read.vm logs -fno longer reprints a short log head as it grows (c77f8d4). Regression test included.snapshot importcloses its archive on cancel (34dd128), like every other streaming verb.e49618d), so an error Firecracker prints before its logger starts is no longer lost.daemon --api-socket-modehelp shows the default as0660(546296c); it printed decimal432.image rmreports what an earlier backend deleted before a later one fails (5e8179a); image gc logs "collected" only when every remover succeeded (17a7a6b);status --eventheader aligns with the rows (943dd87).meta-upgrade.pyrefuses an in-flight convert on json roots too (84a3154), not only on sqlite roots.3726cca): seven read paths returned a variable next to the call that assigns it.go test -racewas red (09ef482): a meta/json test sent on fsnotify's own Errors channel, which kqueue closes behind a pipe the race detector cannot see. The branch is now tested onmeta.Broadcasterwith a synthetic watcher; the json notifier's test-only field is gone.Cuts
Each cut is backed by the existing tests; the tests deleted pinned only the removed branch.
completioncommand replaced by cobra's default (7997087, -23 lines).cocoon completion <shell>keeps working.9e9abc3), VM network resolvers (58be21f), backend constructors (1a19073).ToVMmap clone of a freshly decoded record (39ffbdf);placeScoperead of a scope it just created, one sysfs read per pinned launch (7194f64); tombstonePayload.Kind, written and never read, old rows still decode (d98c18d); sqlite notifier's private cancel context (4483f3f);json.Marshalof a uint64 cursor (acb0b33); reseed's re-inspect of a VM whose result already reports no vsock (e7d6a99).Review and docs
eb5fc2e,26c9118,548871d). Repeated func types named (8b7db6f), FC clone layout (f1699c5).81a4172,96e0a02,86cc6bb): failed-starterrorstate and restore marker, interruptedvm rmvsvm netrecovery, leased vs deleting tombstones, SSE event names, FC clone MAC conflict, FC guest hints printed on every call, fs tag and device id patterns,--dnsseparators, sqlite refusing NFS/CIFS/FUSE roots, v0.7.7 install URLs.contents: readon build, lint and test; deados-image/windowsand.skip-generic-buildbranches removed (b72d776).Withdrawn after review
commitBlobsto delete the boot-file self-heal (~120 lines). Boot files are not fsynced, so a host reboot can recreate the state the heal repairs; the heal stays.Gates
make lint(GOOS linux and darwin):0 issues.×2;make fmt-check: clean.asl ./..., linux and darwin: only the three forwarders already kept in the ledger (instanceStart,pauseVM,quoteIdent).go test -race -count=1 ./...on darwin: all packages ok. CI on Linux: all 39 packages ok (meta/json5.6 s). A local Linux arm64 container timed out inmeta/jsonTestMultiProcessCorrectnesson both origin/master and this branch (container speed, not the change).hypervisor/state_test.gonow assert the refusal they name (7750f7b).Coverage
Known issues kept without code
cocoon gc's 1 h temp sweep can remove an OCI pull work dir whose last layer takes over an hour; raising the default needs a measurement.errorstate to a stopped origin; the restore marker already blocksvm start.