From 1cb860cc823c2fb23863f43d3e412acb2edf5f81 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 6 Jul 2026 18:10:42 +0800 Subject: [PATCH 1/2] review: add godoc to exported types, constructors, and asymmetric method siblings From a full-repo /code audit against the updated SKILL.md. Only exported types, New constructors, and methods whose documented siblings make the omission an inconsistency are covered; interface-implementing methods, consistent accessor/skip-all files, and build-tag stubs are left as-is. --- cmd/snapshot/handler.go | 1 + gc/orchestrator.go | 1 + hypervisor/cloudhypervisor/direct.go | 1 + hypervisor/create.go | 1 + hypervisor/firecracker/direct.go | 1 + hypervisor/snapshot.go | 1 + images/op.go | 1 + lock/flock/flock.go | 1 + metering/capture/capture.go | 2 ++ metering/file/file.go | 1 + metering/stderr/stderr.go | 1 + snapshot/db.go | 1 + storage/json/json.go | 2 ++ 13 files changed, 15 insertions(+) diff --git a/cmd/snapshot/handler.go b/cmd/snapshot/handler.go index 9dc2c9a2..72bdec33 100644 --- a/cmd/snapshot/handler.go +++ b/cmd/snapshot/handler.go @@ -18,6 +18,7 @@ import ( "github.com/cocoonstack/cocoon/types" ) +// Handler groups the snapshot subcommands. type Handler struct { cmdcore.BaseHandler } diff --git a/gc/orchestrator.go b/gc/orchestrator.go index 0703c504..63397922 100644 --- a/gc/orchestrator.go +++ b/gc/orchestrator.go @@ -17,6 +17,7 @@ type Orchestrator struct { modules []runner } +// New returns an Orchestrator with no registered modules. func New() *Orchestrator { return &Orchestrator{} } // Register is package-level because Go methods can't have type params. diff --git a/hypervisor/cloudhypervisor/direct.go b/hypervisor/cloudhypervisor/direct.go index 7312feca..2d88000b 100644 --- a/hypervisor/cloudhypervisor/direct.go +++ b/hypervisor/cloudhypervisor/direct.go @@ -15,6 +15,7 @@ func (ch *CloudHypervisor) DirectClone(ctx context.Context, vmID string, vmCfg * return ch.DirectCloneBase(ctx, vmID, vmCfg, net, snapshotConfig, srcDir, cloneSnapshotFiles, ch.cloneAfterExtract) } +// DirectRestore restores a VM in place from a local snapshot dir. func (ch *CloudHypervisor) DirectRestore(ctx context.Context, vmRef string, vmCfg *types.VMConfig, srcDir, sourceSnapshotID string) (*types.VM, error) { return ch.DirectRestoreSequence(ctx, vmRef, hypervisor.DirectRestoreSpec{ VMCfg: vmCfg, diff --git a/hypervisor/create.go b/hypervisor/create.go index 1fde2129..f9aa55e6 100644 --- a/hypervisor/create.go +++ b/hypervisor/create.go @@ -36,6 +36,7 @@ func (b *Backend) ReserveVM(ctx context.Context, id string, vmCfg *types.VMConfi }) } +// RollbackCreate removes the placeholder record and its name mapping after a failed create. func (b *Backend) RollbackCreate(ctx context.Context, id, name string) { if err := b.DB.Update(ctx, func(idx *VMIndex) error { delete(idx.VMs, id) diff --git a/hypervisor/firecracker/direct.go b/hypervisor/firecracker/direct.go index 82a4e263..51043f27 100644 --- a/hypervisor/firecracker/direct.go +++ b/hypervisor/firecracker/direct.go @@ -13,6 +13,7 @@ func (fc *Firecracker) DirectClone(ctx context.Context, vmID string, vmCfg *type return fc.DirectCloneBase(ctx, vmID, vmCfg, net, snapshotConfig, srcDir, cloneSnapshotFiles, fc.cloneAfterExtract) } +// DirectRestore restores a VM in place from a local snapshot dir. func (fc *Firecracker) DirectRestore(ctx context.Context, vmRef string, vmCfg *types.VMConfig, srcDir, sourceSnapshotID string) (*types.VM, error) { return fc.DirectRestoreSequence(ctx, vmRef, hypervisor.DirectRestoreSpec{ VMCfg: vmCfg, diff --git a/hypervisor/snapshot.go b/hypervisor/snapshot.go index 33191a9f..730654de 100644 --- a/hypervisor/snapshot.go +++ b/hypervisor/snapshot.go @@ -18,6 +18,7 @@ import ( // SnapshotMetaFile is the cocoon-owned sidecar carrying fields the hypervisor's native config can't hold (Role/MountPoint/FSType/DirectIO; FC CPU/Memory). const SnapshotMetaFile = "cocoon.json" +// SnapshotMeta is the schema of the cocoon.json snapshot sidecar. type SnapshotMeta struct { StorageConfigs []*types.StorageConfig `json:"storage_configs"` BootConfig *types.BootConfig `json:"boot_config,omitempty"` diff --git a/images/op.go b/images/op.go index 14405f9f..c4a351c4 100644 --- a/images/op.go +++ b/images/op.go @@ -29,6 +29,7 @@ func (ops Ops[I, E]) Inspect(ctx context.Context, id string) (result *types.Imag return result, err } +// List returns every image in the index. func (ops Ops[I, E]) List(ctx context.Context) (result []*types.Image, err error) { err = ops.Store.With(ctx, func(idx *I) error { result = listImages(ops.Entries(idx), ops.Type, ops.Sizer) diff --git a/lock/flock/flock.go b/lock/flock/flock.go index a6d55bac..ba635b05 100644 --- a/lock/flock/flock.go +++ b/lock/flock/flock.go @@ -21,6 +21,7 @@ type Lock struct { fl *flock.Flock // active flock fd, non-nil while held } +// New returns a Lock guarding path; the lock file is created on first acquire. func New(path string) *Lock { return &Lock{path: path, ch: make(chan struct{}, 1)} } diff --git a/metering/capture/capture.go b/metering/capture/capture.go index 7f8e4ebc..7da7dc2d 100644 --- a/metering/capture/capture.go +++ b/metering/capture/capture.go @@ -11,11 +11,13 @@ import ( var _ metering.Recorder = (*Recorder)(nil) +// Recorder collects emitted entries in memory for test assertions. type Recorder struct { mu sync.Mutex entries []metering.Entry } +// New returns an empty in-memory Recorder. func New() *Recorder { return &Recorder{} } func (r *Recorder) Emit(_ context.Context, e metering.Entry) { diff --git a/metering/file/file.go b/metering/file/file.go index d13c1ef5..b508d294 100644 --- a/metering/file/file.go +++ b/metering/file/file.go @@ -19,6 +19,7 @@ type Recorder struct { f *os.File } +// New opens or creates the append-only ledger at path. func New(path string) (*Recorder, error) { f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) //nolint:gosec if err != nil { diff --git a/metering/stderr/stderr.go b/metering/stderr/stderr.go index cc56989c..2c974f8c 100644 --- a/metering/stderr/stderr.go +++ b/metering/stderr/stderr.go @@ -17,6 +17,7 @@ type Recorder struct { out io.Writer } +// New returns a Recorder that writes entries to os.Stderr. func New() *Recorder { return &Recorder{out: os.Stderr} } func (r *Recorder) Emit(_ context.Context, e metering.Entry) { diff --git a/snapshot/db.go b/snapshot/db.go index 84231f9a..ae41c8cc 100644 --- a/snapshot/db.go +++ b/snapshot/db.go @@ -8,6 +8,7 @@ import ( "github.com/cocoonstack/cocoon/utils" ) +// ErrNotFound is returned when a snapshot ref matches no record. var ErrNotFound = errors.New("snapshot not found") // SnapshotRecord is the persisted record for a single snapshot. diff --git a/storage/json/json.go b/storage/json/json.go index ae6c8ef1..8569d771 100644 --- a/storage/json/json.go +++ b/storage/json/json.go @@ -14,11 +14,13 @@ import ( var _ storage.Store[struct{}] = (*Store[struct{}])(nil) +// Store is a lock-guarded JSON file holding one value of type T. type Store[T any] struct { filePath string locker lock.Locker } +// New returns a Store backed by filePath and guarded by locker. func New[T any](filePath string, locker lock.Locker) *Store[T] { return &Store[T]{filePath: filePath, locker: locker} } From c64b8d9905ddab9faeecd42e375b5f78370c837c Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 6 Jul 2026 18:18:46 +0800 Subject: [PATCH 2/2] review: normalize declaration layout and drop section-label comments Full-repo /code audit follow-up; layout-only, zero behavior change: - metering: place compile-time interface checks above their implementing types (Entry, NopRecorder) instead of a shared top-of-file var block - cloudhypervisor/utils: move chMemoryRestoreMode below const/var (imports -> const -> var -> types) - oci/cloudimg image: keep imageIndex and imageEntry each adjacent to their own method set; leaf/vocabulary types trail - types/vm: move VMConfig.Validate next to VMConfig (was split off by the whole VM type in between) - cloudhypervisor/clone_test: drop bare function-name section labels --- hypervisor/cloudhypervisor/clone_test.go | 8 --- hypervisor/cloudhypervisor/utils.go | 6 +-- images/cloudimg/image.go | 14 +++--- images/oci/image.go | 30 +++++------ metering/metering.go | 9 ++-- types/vm.go | 64 ++++++++++++------------ 6 files changed, 61 insertions(+), 70 deletions(-) diff --git a/hypervisor/cloudhypervisor/clone_test.go b/hypervisor/cloudhypervisor/clone_test.go index e7aeff35..1662efcb 100644 --- a/hypervisor/cloudhypervisor/clone_test.go +++ b/hypervisor/cloudhypervisor/clone_test.go @@ -165,8 +165,6 @@ func TestPatchCHConfig_DiskCountMismatch(t *testing.T) { } } -// updateCOWPath - func TestUpdateCOWPath_OCI(t *testing.T) { configs := []*types.StorageConfig{ {Path: "/old/layer.erofs", RO: true, Serial: "layer0", Role: types.StorageRoleLayer}, @@ -210,8 +208,6 @@ func TestUpdateCOWPath_Cloudimg(t *testing.T) { } } -// rebuildBootConfig - func TestRebuildBootConfig(t *testing.T) { t.Run("nil_payload", func(t *testing.T) { cfg := &chVMConfig{} @@ -258,8 +254,6 @@ func TestRebuildBootConfig(t *testing.T) { }) } -// patchStateJSON - func TestPatchStateJSON(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "state.json") @@ -628,5 +622,3 @@ func basePatchOpts() *patchOptions { directBoot: true, } } - -// patchCHConfig diff --git a/hypervisor/cloudhypervisor/utils.go b/hypervisor/cloudhypervisor/utils.go index abeea4d8..04ad57b5 100644 --- a/hypervisor/cloudhypervisor/utils.go +++ b/hypervisor/cloudhypervisor/utils.go @@ -22,9 +22,6 @@ import ( "github.com/cocoonstack/cocoon/utils" ) -// chMemoryRestoreMode controls how CH restores guest memory from a snapshot. -type chMemoryRestoreMode string - const ( pidFileName = "ch.pid" cmdlineFileName = "cmdline" @@ -41,6 +38,9 @@ const ( var runtimeFiles = []string{hypervisor.APISocketName, pidFileName, hypervisor.ConsoleSockName, cmdlineFileName, hypervisor.VsockSockName} +// chMemoryRestoreMode controls how CH restores guest memory from a snapshot. +type chMemoryRestoreMode string + type chRestoreConfig struct { SourceURL string `json:"source_url"` MemoryRestoreMode chMemoryRestoreMode `json:"memory_restore_mode,omitempty"` diff --git a/images/cloudimg/image.go b/images/cloudimg/image.go index d73805d4..45525714 100644 --- a/images/cloudimg/image.go +++ b/images/cloudimg/image.go @@ -10,13 +10,6 @@ type imageIndex struct { images.Index[imageEntry] } -type imageEntry struct { - Ref string `json:"ref"` - ContentSum images.Digest `json:"content_sum"` - Size int64 `json:"size"` - CreatedAt time.Time `json:"created_at"` -} - // Lookup finds an entry by URL or content digest. func (idx *imageIndex) Lookup(id string) (string, *imageEntry, bool) { if entry, ok := idx.Images[id]; ok && entry != nil { @@ -34,6 +27,13 @@ func (idx *imageIndex) LookupRefs(id string) []string { return images.LookupRefs(idx.Images, id) } +type imageEntry struct { + Ref string `json:"ref"` + ContentSum images.Digest `json:"content_sum"` + Size int64 `json:"size"` + CreatedAt time.Time `json:"created_at"` +} + func (e imageEntry) EntryID() string { return e.ContentSum.String() } func (e imageEntry) EntryRef() string { return e.Ref } func (e imageEntry) EntryCreatedAt() time.Time { return e.CreatedAt } diff --git a/images/oci/image.go b/images/oci/image.go index cd13d545..cc587f2b 100644 --- a/images/oci/image.go +++ b/images/oci/image.go @@ -12,21 +12,6 @@ type imageIndex struct { images.Index[imageEntry] } -// Paths derive from digests at runtime; not stored. -type imageEntry struct { - Ref string `json:"ref"` - ManifestDigest images.Digest `json:"manifest_digest"` - Layers []layerEntry `json:"layers"` - KernelLayer images.Digest `json:"kernel_layer"` - InitrdLayer images.Digest `json:"initrd_layer"` - Size int64 `json:"size"` - CreatedAt time.Time `json:"created_at"` -} - -type layerEntry struct { - Digest images.Digest `json:"digest"` -} - // Lookup finds an entry by ref (exact or normalized) or manifest digest. func (idx *imageIndex) Lookup(id string) (string, *imageEntry, bool) { if entry, ok := idx.Images[id]; ok && entry != nil { @@ -56,6 +41,17 @@ func (idx *imageIndex) LookupRefs(id string) []string { }) } +// Paths derive from digests at runtime; not stored. +type imageEntry struct { + Ref string `json:"ref"` + ManifestDigest images.Digest `json:"manifest_digest"` + Layers []layerEntry `json:"layers"` + KernelLayer images.Digest `json:"kernel_layer"` + InitrdLayer images.Digest `json:"initrd_layer"` + Size int64 `json:"size"` + CreatedAt time.Time `json:"created_at"` +} + func (e imageEntry) EntryID() string { return e.ManifestDigest.String() } func (e imageEntry) EntryRef() string { return e.Ref } func (e imageEntry) EntryCreatedAt() time.Time { return e.CreatedAt } @@ -67,3 +63,7 @@ func (e imageEntry) DigestHexes() []string { } return hexes } + +type layerEntry struct { + Digest images.Digest `json:"digest"` +} diff --git a/metering/metering.go b/metering/metering.go index 6d7ee5fc..76f384e0 100644 --- a/metering/metering.go +++ b/metering/metering.go @@ -27,11 +27,6 @@ const ( ReasonSnapRemove Reason = "snap-rm" ) -var ( - _ io.WriterTo = Entry{} - _ Recorder = NopRecorder{} -) - // Kind identifies a lifecycle endpoint; downstream pairs *.start with *.stop by id. type Kind string @@ -45,6 +40,8 @@ type Shape struct { StorageBytes int64 `json:"storage_bytes,omitempty"` } +var _ io.WriterTo = Entry{} + // Entry is one append-only lifecycle event. type Entry struct { Kind Kind `json:"kind"` @@ -73,6 +70,8 @@ type Recorder interface { Emit(context.Context, Entry) } +var _ Recorder = NopRecorder{} + // NopRecorder discards every entry; zero value is usable. type NopRecorder struct{} diff --git a/types/vm.go b/types/vm.go index c52d8b84..be7f9ac7 100644 --- a/types/vm.go +++ b/types/vm.go @@ -36,6 +36,38 @@ type VMConfig struct { DataDisks []DataDiskSpec `json:"-"` // populated from --data-disk; consumed by Create } +// Validate checks that VMConfig fields are within acceptable ranges. +func (cfg *VMConfig) Validate() error { + if cfg.Name == "" { + return fmt.Errorf("vm name cannot be empty") + } + if !validName.MatchString(cfg.Name) { + return fmt.Errorf("vm name %q is invalid: must match %s (max 63 chars)", cfg.Name, validName.String()) + } + if cfg.CPU <= 0 { + return fmt.Errorf("--cpu must be at least 1, got %d", cfg.CPU) + } + if cfg.Memory < 512<<20 { + return fmt.Errorf("--memory must be at least 512M, got %d", cfg.Memory) + } + if cfg.Storage < 10<<30 { + return fmt.Errorf("--storage must be at least 10G, got %d", cfg.Storage) + } + if cfg.QueueSize < 0 { + return fmt.Errorf("--queue-size must be non-negative, got %d", cfg.QueueSize) + } + if cfg.DiskQueueSize < 0 { + return fmt.Errorf("--disk-queue-size must be non-negative, got %d", cfg.DiskQueueSize) + } + if cfg.User != "" && !validUsername.MatchString(cfg.User) { + return fmt.Errorf("--user %q is invalid: must be a lowercase Linux username (letters, digits, underscores, hyphens)", cfg.User) + } + if cfg.Password != "" && shellUnsafe.MatchString(cfg.Password) { + return fmt.Errorf("--password contains unsafe shell or YAML characters") + } + return nil +} + // NetSetup is the VM's host networking state, embedded in VM and reused as the initNetwork → hypervisor handoff. type NetSetup struct { NetBackend string `json:"net_backend,omitempty"` @@ -72,38 +104,6 @@ type VM struct { StoppedAt *time.Time `json:"stopped_at,omitempty"` } -// Validate checks that VMConfig fields are within acceptable ranges. -func (cfg *VMConfig) Validate() error { - if cfg.Name == "" { - return fmt.Errorf("vm name cannot be empty") - } - if !validName.MatchString(cfg.Name) { - return fmt.Errorf("vm name %q is invalid: must match %s (max 63 chars)", cfg.Name, validName.String()) - } - if cfg.CPU <= 0 { - return fmt.Errorf("--cpu must be at least 1, got %d", cfg.CPU) - } - if cfg.Memory < 512<<20 { - return fmt.Errorf("--memory must be at least 512M, got %d", cfg.Memory) - } - if cfg.Storage < 10<<30 { - return fmt.Errorf("--storage must be at least 10G, got %d", cfg.Storage) - } - if cfg.QueueSize < 0 { - return fmt.Errorf("--queue-size must be non-negative, got %d", cfg.QueueSize) - } - if cfg.DiskQueueSize < 0 { - return fmt.Errorf("--disk-queue-size must be non-negative, got %d", cfg.DiskQueueSize) - } - if cfg.User != "" && !validUsername.MatchString(cfg.User) { - return fmt.Errorf("--user %q is invalid: must be a lowercase Linux username (letters, digits, underscores, hyphens)", cfg.User) - } - if cfg.Password != "" && shellUnsafe.MatchString(cfg.Password) { - return fmt.Errorf("--password contains unsafe shell or YAML characters") - } - return nil -} - // ResolvedNetnsPath returns NetnsPath, with NIC[0] fallback. func (v *VM) ResolvedNetnsPath() string { if v == nil {