From cf2db9dad307e57d36fb70b227e88bc4866013ae Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 2 Jul 2026 11:16:39 +0800 Subject: [PATCH] refactor: whole-project /code audit round 2 (fixes + comment tightening) Line-by-line audit of all 236 Go files against the /code rulebook, findings adversarially verified before fixing. Correctness-adjacent: - utils.ResolveRef prefix path could resolve a null index entry and panic downstream (exact/name paths already nil-checked); skip nil entries - types.VM Resolved* methods deref NetworkConfigs[0] unguarded; add nil-safe firstNIC() accessor - snapshot export --to-dir flag help was mangled by pflag backquote handling - cmd/vm/status watch loop and cmd/vm run cloneFromDir copy pointers unguarded - firecracker console-relay failure now logs the actual error - CNI conflist load errors are no longer silently dropped: stashed and surfaced through errNoConflist at the point of failure Consistency/modernisms: - hand-rolled loops -> slices.ContainsFunc/DeleteFunc/Clone/Concat, maps.Copy, utils.MapValues reuse - qcow2/gzip magic bytes deduped into utils/magic.go (3 copies -> 1) - dead network.ErrNotFound sentinel removed; metering const/var block order; progress exported-above ordering; cmd.vm.* log tag consistency; debug.go verb-less Printf -> Print/Println; BlobExt instead of ".qcow2" Comments: 131 tightened to 1-2 lines (net -138 lines project-wide), dropping restatement/narration while keeping constraint-bearing WHYs; two stale comments corrected against actual behavior (MountSpec renders verbatim, sparse fallback triggers on empty files). --- cmd/core/ensure_image_test.go | 6 +-- cmd/core/helpers.go | 3 +- cmd/core/metering.go | 2 +- cmd/images/handler_test.go | 10 ++--- cmd/images/import.go | 7 ++-- cmd/snapshot/commands.go | 2 +- cmd/snapshot/handler.go | 11 ++--- cmd/vm/debug.go | 28 ++++++------- cmd/vm/exec_test.go | 3 +- cmd/vm/lifecycle.go | 2 +- cmd/vm/run.go | 3 ++ cmd/vm/status.go | 15 +++---- config/config.go | 35 ++++++---------- extend/fs/fs.go | 3 +- extend/vfio/vfio.go | 3 +- gc/module.go | 2 +- gc/refs.go | 12 +++--- gc/runner.go | 3 +- hypervisor/cloudhypervisor/api.go | 4 +- hypervisor/cloudhypervisor/args.go | 3 -- hypervisor/cloudhypervisor/cloudhypervisor.go | 1 - hypervisor/cloudhypervisor/console.go | 1 - hypervisor/cloudhypervisor/helper.go | 13 +++--- hypervisor/cloudhypervisor/utils.go | 6 +-- hypervisor/firecracker/api.go | 2 - hypervisor/firecracker/relay.go | 7 +--- hypervisor/firecracker/start.go | 10 ++--- hypervisor/inspect.go | 10 ++--- hypervisor/state_test.go | 3 -- hypervisor/utils.go | 3 +- hypervisor/utils_test.go | 3 +- images/cloudimg/gc.go | 2 +- images/cloudimg/inspect.go | 41 ++++++++----------- images/cloudimg/pull.go | 2 +- images/gc.go | 3 +- images/index.go | 7 +--- images/oci/pull.go | 6 +-- images/op.go | 3 +- main.go | 3 +- metadata/fat12.go | 1 - metadata/metadata.go | 3 +- metadata/metadata_test.go | 4 +- metering/metering.go | 22 +++++----- metering/metering_test.go | 3 +- network/bridge/gc_linux.go | 2 - network/cni/cni.go | 17 ++++---- network/cni/db.go | 7 +--- network/cni/gc.go | 4 +- network/network.go | 5 +-- progress/progress.go | 12 +++--- snapshot/localfile/export.go | 3 +- snapshot/localfile/gc.go | 2 +- snapshot/localfile/import.go | 6 +-- snapshot/localfile/localfile.go | 2 +- snapshot/localfile/localfile_test.go | 6 +-- snapshot/snapshot.go | 3 +- storage/json/json_test.go | 5 --- storage/storage.go | 9 ++-- types/boot.go | 3 +- types/network.go | 3 +- types/storage.go | 8 ++-- types/vm.go | 33 ++++++++------- utils/atomic.go | 3 +- utils/atomic_test.go | 1 - utils/file.go | 3 +- utils/file_test.go | 14 +++---- utils/http_test.go | 3 +- utils/hugepages.go | 2 +- utils/magic.go | 10 +++++ utils/process_test.go | 15 +------ utils/resolve.go | 5 +-- utils/sparse_linux_test.go | 12 ------ utils/sparse_test.go | 6 +-- utils/tar_sparse_linux.go | 2 +- utils/tar_test.go | 28 ++++--------- 75 files changed, 212 insertions(+), 338 deletions(-) create mode 100644 utils/magic.go diff --git a/cmd/core/ensure_image_test.go b/cmd/core/ensure_image_test.go index 5dde2b4d..b4c801bb 100644 --- a/cmd/core/ensure_image_test.go +++ b/cmd/core/ensure_image_test.go @@ -42,8 +42,7 @@ func (f *fakeImageBackend) Config(context.Context, []*types.VMConfig) ([][]*type return nil, nil, nil } -// Regression guard for issue 37: pinned digest with no local hit must force-pull -// to bypass cloudimg's URL-keyed cache. +// Issue 37 regression guard: a pinned digest with no local hit must force-pull past cloudimg's URL-keyed cache. func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) { const ( url = "https://epoch.example/dl/simular/win11" @@ -98,8 +97,7 @@ func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) { } } -// A cloudimg ref without an http(s) scheme reaching Pull surfaces as -// `unsupported protocol scheme` from http.Get; the shape guard short-circuits. +// A non-http(s) cloudimg ref reaching Pull would surface http.Get's `unsupported protocol scheme`; the shape guard short-circuits first. func TestEnsureImage_SkipsBadShape(t *testing.T) { tests := []struct { name string diff --git a/cmd/core/helpers.go b/cmd/core/helpers.go index 867607d7..a61b25ca 100644 --- a/cmd/core/helpers.go +++ b/cmd/core/helpers.go @@ -98,8 +98,7 @@ func EnsureImage(ctx context.Context, backends []imagebackend.Images, vmCfg *typ if img != nil { return // exact image version exists locally } - // Pull by digest reference when available — ensures we get the exact - // version recorded at snapshot time, not whatever the tag points to now. + // Pull by digest when available — the tag may point at a different manifest than at snapshot time. pullRef := digestPullRef(vmCfg.Image, vmCfg.ImageDigest, vmCfg.ImageType) if shapeErr := validateRefShape(pullRef, vmCfg.ImageType); shapeErr != nil { logger.Warnf(ctx, "skipping auto-pull of %s: %v — pre-pull manually if missing", pullRef, shapeErr) diff --git a/cmd/core/metering.go b/cmd/core/metering.go index 86e038be..2d0a4b41 100644 --- a/cmd/core/metering.go +++ b/cmd/core/metering.go @@ -41,7 +41,7 @@ func buildRecorder(ctx context.Context, conf *config.Config) metering.Recorder { case "stderr": return meteringstderr.New() default: - log.WithFunc("core.MeteringRecorder").Warnf(ctx, "unknown metering backend %q; using nop", conf.Metering.Backend) + log.WithFunc("core.buildRecorder").Warnf(ctx, "unknown metering backend %q; using nop", conf.Metering.Backend) return metering.NopRecorder{} } } diff --git a/cmd/images/handler_test.go b/cmd/images/handler_test.go index 3ec015f0..2a0b4828 100644 --- a/cmd/images/handler_test.go +++ b/cmd/images/handler_test.go @@ -8,9 +8,9 @@ import ( "os" "path/filepath" "testing" -) -var qcow2Magic = []byte{'Q', 'F', 'I', 0xfb} + "github.com/cocoonstack/cocoon/utils" +) func gzipWrap(t *testing.T, data []byte) []byte { t.Helper() @@ -55,7 +55,7 @@ func writeTempFile(t *testing.T, dir, name string, data []byte) string { } func TestDetectReader(t *testing.T) { - qcow2Data := append(qcow2Magic, make([]byte, 100)...) + qcow2Data := append(utils.Qcow2Magic, make([]byte, 100)...) tarData := []byte("this is a tar-like stream of data, not really tar but not qcow2 either") tests := []struct { @@ -155,7 +155,7 @@ func TestDetectReader_GzipPreservesFullContent(t *testing.T) { func TestDetectLocalImportSource(t *testing.T) { dir := t.TempDir() - qcow2Path := writeTempFile(t, dir, "image.qcow2", append(qcow2Magic, make([]byte, 32)...)) + qcow2Path := writeTempFile(t, dir, "image.qcow2", append(utils.Qcow2Magic, make([]byte, 32)...)) tarPath := writeTempFile(t, dir, "image.tar", tarWrap(t, "payload.txt", []byte("payload"))) gzipTarPath := writeTempFile(t, dir, "image.tar.gz", gzipWrap(t, tarWrap(t, "payload.txt", []byte("payload")))) invalidPath := writeTempFile(t, dir, "invalid.bin", []byte("not an archive")) @@ -193,7 +193,7 @@ func TestDetectLocalImportSource(t *testing.T) { func TestPlanLocalImportPreservesAllFiles(t *testing.T) { dir := t.TempDir() - part1 := writeTempFile(t, dir, "part-1.qcow2", append(qcow2Magic, make([]byte, 8)...)) + part1 := writeTempFile(t, dir, "part-1.qcow2", append(utils.Qcow2Magic, make([]byte, 8)...)) part2 := writeTempFile(t, dir, "part-2.qcow2", []byte("second-part")) plan, err := planLocalImport([]string{part1, part2}) diff --git a/cmd/images/import.go b/cmd/images/import.go index 55054f80..9a51f858 100644 --- a/cmd/images/import.go +++ b/cmd/images/import.go @@ -20,6 +20,7 @@ import ( "github.com/cocoonstack/cocoon/progress" cloudimgProgress "github.com/cocoonstack/cocoon/progress/cloudimg" ociProgress "github.com/cocoonstack/cocoon/progress/oci" + "github.com/cocoonstack/cocoon/utils" ) func (h Handler) Import(cmd *cobra.Command, args []string) error { @@ -226,10 +227,10 @@ func detectLocalImportSource(filePath string) (importSourceKind, error) { return 0, fmt.Errorf("peek %s: %w", filePath, readErr) } - if n >= 2 && bytes.Equal(magic[:2], []byte{0x1f, 0x8b}) { + if n >= 2 && bytes.Equal(magic[:2], utils.GzipMagic) { return importSourceStream, nil } - if n >= 4 && bytes.Equal(magic[:4], []byte{'Q', 'F', 'I', 0xfb}) { + if n >= 4 && bytes.Equal(magic[:4], utils.Qcow2Magic) { return importSourceQcow2, nil } @@ -270,7 +271,7 @@ func detectReader(r io.Reader) (io.Reader, imageType, func(), error) { return nil, 0, nil, fmt.Errorf("peek content: %w", err) } - if cpeek[0] == 'Q' && cpeek[1] == 'F' && cpeek[2] == 'I' && cpeek[3] == 0xfb { + if bytes.HasPrefix(cpeek, utils.Qcow2Magic) { return inner, imageTypeQcow2, cleanup, nil } diff --git a/cmd/snapshot/commands.go b/cmd/snapshot/commands.go index 72a47e0f..e0fcdebe 100644 --- a/cmd/snapshot/commands.go +++ b/cmd/snapshot/commands.go @@ -61,7 +61,7 @@ func Command(h Actions) *cobra.Command { } exportCmd.Flags().StringP("output", "o", "", "output file path (default: .tar)") exportCmd.Flags().Bool("gzip", false, "compress output with gzip") - exportCmd.Flags().String("to-dir", "", "export into a directory (must be empty/absent) instead of a tar; pairs with `vm clone --from-dir`") + exportCmd.Flags().String("to-dir", "", "export into a directory (must be empty/absent) instead of a tar; pairs with 'vm clone --from-dir'") exportCmd.MarkFlagsMutuallyExclusive("to-dir", "output") exportCmd.MarkFlagsMutuallyExclusive("to-dir", "gzip") diff --git a/cmd/snapshot/handler.go b/cmd/snapshot/handler.go index da428104..7e7505e3 100644 --- a/cmd/snapshot/handler.go +++ b/cmd/snapshot/handler.go @@ -110,13 +110,10 @@ func (h Handler) List(cmd *cobra.Command, _ []string) error { } if filterIDs != nil { - filtered := snapshots[:0] - for _, s := range snapshots { - if _, ok := filterIDs[s.ID]; ok { - filtered = append(filtered, s) - } - } - snapshots = filtered + snapshots = slices.DeleteFunc(snapshots, func(s *types.Snapshot) bool { + _, ok := filterIDs[s.ID] + return !ok + }) } if len(snapshots) == 0 { diff --git a/cmd/vm/debug.go b/cmd/vm/debug.go index eea6992a..014d99ba 100644 --- a/cmd/vm/debug.go +++ b/cmd/vm/debug.go @@ -109,39 +109,38 @@ func printFCDebug(configs []*types.StorageConfig, boot *types.BootConfig, vmCfg fmt.Println("# Configure via REST API (use curl or similar):") sock := fmt.Sprintf("/tmp/fc-%s.sock", vmCfg.Name) - fmt.Printf("# 1. Machine config\n") + fmt.Println("# 1. Machine config") fmt.Printf("curl --unix-socket %s -X PUT http://localhost/machine-config \\\n", sock) fmt.Printf(" -d '{\"vcpu_count\": %d, \"mem_size_mib\": %d}'\n", vmCfg.CPU, memMiB) fmt.Println() - fmt.Printf("# 2. Boot source\n") + fmt.Println("# 2. Boot source") fmt.Printf("curl --unix-socket %s -X PUT http://localhost/boot-source \\\n", sock) fmt.Printf(" -d '{\"kernel_image_path\": \"%s\", \"initrd_path\": \"%s\", \"boot_args\": \"%s\"}'\n", boot.KernelPath, boot.InitrdPath, cmdline) fmt.Println() - fmt.Printf("# 3. Drives\n") + fmt.Println("# 3. Drives") for i, sc := range configs { fmt.Printf("curl --unix-socket %s -X PUT http://localhost/drives/drive_%d \\\n", sock, i) fmt.Printf(" -d '{\"drive_id\": \"drive_%d\", \"path_on_host\": \"%s\", \"is_root_device\": false, \"is_read_only\": %t}'\n", i, sc.Path, sc.RO) } - // COW drive fmt.Printf("curl --unix-socket %s -X PUT http://localhost/drives/drive_%d \\\n", sock, len(configs)) fmt.Printf(" -d '{\"drive_id\": \"drive_%d\", \"path_on_host\": \"%s\", \"is_root_device\": false, \"is_read_only\": false}'\n", len(configs), cowPath) fmt.Println() if size, ok := hypervisor.BalloonSize(vmCfg.Memory, vmCfg.Windows); ok { - fmt.Printf("# 4. Balloon\n") + fmt.Println("# 4. Balloon") fmt.Printf("curl --unix-socket %s -X PUT http://localhost/balloon \\\n", sock) fmt.Printf(" -d '{\"amount_mib\": %d, \"deflate_on_oom\": true, \"free_page_reporting\": true}'\n", size>>20) //nolint:mnd fmt.Println() } - fmt.Printf("# 5. Start\n") + fmt.Println("# 5. Start") fmt.Printf("curl --unix-socket %s -X PUT http://localhost/actions \\\n", sock) - fmt.Printf(" -d '{\"action_type\": \"InstanceStart\"}'\n") + fmt.Println(" -d '{\"action_type\": \"InstanceStart\"}'") } func buildCHDebugSpec(cmd *cobra.Command, storageConfigs []*types.StorageConfig, boot *types.BootConfig, vmCfg *types.VMConfig) chDebugSpec { @@ -149,8 +148,7 @@ func buildCHDebugSpec(cmd *cobra.Command, storageConfigs []*types.StorageConfig, balloon, _ := cmd.Flags().GetInt("balloon") cowPath, _ := cmd.Flags().GetString("cow") chBin, _ := cmd.Flags().GetString("ch") - // Mirror runtime gating: Windows / sub-MinBalloon VMs never get balloon, - // even if the user passed --balloon, so debug output stays truthful. + // Mirror runtime gating: Windows / sub-MinBalloon VMs never get balloon even with --balloon, so debug output stays truthful. size, ok := hypervisor.BalloonSize(vmCfg.Memory, vmCfg.Windows) switch { case !ok: @@ -196,11 +194,11 @@ func printCHDebug(s chDebugSpec) { fmt.Printf("%s \\\n", s.CHBin) fmt.Printf(" --kernel %s \\\n", s.Boot.KernelPath) fmt.Printf(" --initramfs %s \\\n", s.Boot.InitrdPath) - fmt.Printf(" --disk") + fmt.Print(" --disk") for _, d := range diskArgs { fmt.Printf(" \\\n \"%s\"", d) } - fmt.Printf(" \\\n") + fmt.Print(" \\\n") fmt.Printf(" --cmdline \"%s\" \\\n", cmdline) } else { if s.CowPath == "" { @@ -216,7 +214,7 @@ func printCHDebug(s chDebugSpec) { fmt.Printf("# Launch VM: %s (image: %s, boot: UEFI firmware)\n", s.VMCfg.Name, s.VMCfg.Image) fmt.Printf("%s \\\n", s.CHBin) fmt.Printf(" --firmware %s \\\n", s.Boot.FirmwarePath) - fmt.Printf(" --disk \\\n") + fmt.Print(" --disk \\\n") diskArgs := cloudhypervisor.DebugDiskCLIArgs([]*types.StorageConfig{{Path: s.CowPath, RO: false}}, cpu, diskQueueSize, noDirectIO) fmt.Printf(" \"%s\" \\\n", diskArgs[0]) } @@ -234,10 +232,10 @@ func printCommonCHArgs(s chDebugSpec) { } fmt.Printf(" --cpus boot=%d,max=%d%s \\\n", s.VMCfg.CPU, s.MaxCPU, cpuExtra) fmt.Printf(" --memory size=%dM%s \\\n", s.VMCfg.Memory>>20, memExtra) //nolint:mnd - fmt.Printf(" --rng src=/dev/urandom \\\n") + fmt.Print(" --rng src=/dev/urandom \\\n") if s.Balloon > 0 { fmt.Printf(" --balloon size=%dM,deflate_on_oom=on,free_page_reporting=on \\\n", s.Balloon) } - fmt.Printf(" --watchdog \\\n") - fmt.Printf(" --serial tty --console off\n") + fmt.Print(" --watchdog \\\n") + fmt.Println(" --serial tty --console off") } diff --git a/cmd/vm/exec_test.go b/cmd/vm/exec_test.go index 6c45b1d9..7f60e38c 100644 --- a/cmd/vm/exec_test.go +++ b/cmd/vm/exec_test.go @@ -77,8 +77,7 @@ func TestReadHybridVsockReply(t *testing.T) { } } -// TestDialHybridVsock_ConnectHandshake spins up an in-process listener that -// speaks the CH/FC hybrid vsock dialect (CONNECT \n → OK \n). +// TestDialHybridVsock_ConnectHandshake drives the CH/FC hybrid vsock dialect (CONNECT \n → OK \n) against an in-process listener. func TestDialHybridVsock_ConnectHandshake(t *testing.T) { // macOS caps unix socket paths at ~104 bytes, so t.TempDir() (long // /var/folders/... path) can overflow. Use os.CreateTemp + immediate unlink. diff --git a/cmd/vm/lifecycle.go b/cmd/vm/lifecycle.go index 6b5b2e93..1d0199a4 100644 --- a/cmd/vm/lifecycle.go +++ b/cmd/vm/lifecycle.go @@ -343,7 +343,7 @@ func finishRoutedCmd(ctx context.Context, cmd *cobra.Command, logTag, name, past } func batchRoutedCmd(ctx context.Context, cmd *cobra.Command, name, pastTense string, routed map[hypervisor.Hypervisor][]string, fn func(hypervisor.Hypervisor, []string) ([]string, error)) error { - logTag := "cmd." + name + logTag := "cmd.vm." + name allDone, lastErr := runRoutedLoop(ctx, logTag, pastTense, cmdcore.WantJSON(cmd), routed, fn) return finishRoutedCmd(ctx, cmd, logTag, name, pastTense, allDone, lastErr) } diff --git a/cmd/vm/run.go b/cmd/vm/run.go index 1e75e663..afd26346 100644 --- a/cmd/vm/run.go +++ b/cmd/vm/run.go @@ -251,6 +251,9 @@ func (h Handler) cloneFromDir(ctx context.Context, cmd *cobra.Command, conf *con if err != nil { return fmt.Errorf("load envelope: %w", err) } + if conf == nil { + return fmt.Errorf("nil config") + } // Local copy keeps backend flip from leaking to the caller's shared *config.Config. localConf := *conf if cfg.Hypervisor != "" { diff --git a/cmd/vm/status.go b/cmd/vm/status.go index 0dadf1c2..2a1a0a77 100644 --- a/cmd/vm/status.go +++ b/cmd/vm/status.go @@ -242,6 +242,9 @@ func statusEventDiffLoop(ctx context.Context, hypers []hypervisor.Hypervisor, fi vms := listAndFilter(ctx, hypers, filters) curr := make(map[string]entry, len(vms)) for _, vm := range vms { + if vm == nil { + continue + } state := cmdcore.ReconcileState(vm) vmCopy := *vm vmCopy.State = types.VMState(state) @@ -315,15 +318,9 @@ func applyFilters(vms []*types.VM, filters []string) []*types.VM { } func matchesFilter(vm *types.VM, filters []string) bool { - for _, f := range filters { - if vm.ID == f || vm.Config.Name == f { - return true - } - if len(f) >= 3 && strings.HasPrefix(vm.ID, f) { - return true - } - } - return false + return slices.ContainsFunc(filters, func(f string) bool { + return vm.ID == f || vm.Config.Name == f || (len(f) >= 3 && strings.HasPrefix(vm.ID, f)) + }) } func snapshotAll(vms []*types.VM) []vmSnapshot { diff --git a/config/config.go b/config/config.go index e921632c..b2e7154c 100644 --- a/config/config.go +++ b/config/config.go @@ -19,44 +19,33 @@ type HypervisorType string // Config holds global Cocoon configuration. type Config struct { - // RootDir is the base directory for persistent data (images, firmware, VM DB). - // Env: COCOON_ROOT_DIR. Default: /var/lib/cocoon. + // RootDir: persistent data (images, firmware, VM DB). Env: COCOON_ROOT_DIR. Default: /var/lib/cocoon. RootDir string `json:"root_dir" mapstructure:"root_dir"` // RunDir: ephemeral runtime state (PID files, sockets). Env: COCOON_RUN_DIR. Default: /var/lib/cocoon/run. RunDir string `json:"run_dir" mapstructure:"run_dir"` - // LogDir is the base directory for VM and process logs. - // Env: COCOON_LOG_DIR. Default: /var/log/cocoon. + // LogDir: VM and process logs. Env: COCOON_LOG_DIR. Default: /var/log/cocoon. LogDir string `json:"log_dir" mapstructure:"log_dir"` - // CHBinary is the path or name of the cloud-hypervisor executable. - // Default: "cloud-hypervisor". + // CHBinary: path or name of the cloud-hypervisor executable. Default: "cloud-hypervisor". CHBinary string `json:"ch_binary" mapstructure:"ch_binary"` - // FCBinary is the path or name of the firecracker executable. - // Default: "firecracker". + // FCBinary: path or name of the firecracker executable. Default: "firecracker". FCBinary string `json:"fc_binary" mapstructure:"fc_binary"` - // UseFirecracker selects Firecracker as the hypervisor backend. - // Set via --fc flag. Default: false (use Cloud Hypervisor). + // UseFirecracker selects the Firecracker backend (--fc flag). Default: false (Cloud Hypervisor). UseFirecracker bool `json:"use_firecracker,omitempty" mapstructure:"use_firecracker"` // StopTimeoutSeconds: guest ACPI grace before SIGTERM/SIGKILL escalation. Default: 30. StopTimeoutSeconds int `json:"stop_timeout_seconds" mapstructure:"stop_timeout_seconds"` - // PoolSize is the goroutine pool size for concurrent operations. - // Defaults to runtime.NumCPU() if zero. + // PoolSize: goroutine pool size for concurrent operations; 0 = runtime.NumCPU(). PoolSize int `json:"pool_size" mapstructure:"pool_size"` - // CNIConfDir is the directory for CNI plugin configuration files. - // Default: /etc/cni/net.d. + // CNIConfDir: CNI plugin configuration dir. Default: /etc/cni/net.d. CNIConfDir string `json:"cni_conf_dir" mapstructure:"cni_conf_dir"` - // CNIBinDir is the directory for CNI plugin binaries. - // Default: /opt/cni/bin. + // CNIBinDir: CNI plugin binary dir. Default: /opt/cni/bin. CNIBinDir string `json:"cni_bin_dir" mapstructure:"cni_bin_dir"` // DNS: comma/semicolon-separated DNS servers injected into VM net config. Env: COCOON_DNS. Default: "8.8.8.8,1.1.1.1". DNS string `json:"dns" mapstructure:"dns"` - // SocketWaitTimeoutSeconds is how long to wait for the CH API socket - // after process start. Default: 5. Increase for slow storage. + // SocketWaitTimeoutSeconds: wait for the CH API socket after start. Default: 5; increase for slow storage. SocketWaitTimeoutSeconds int `json:"socket_wait_timeout_seconds" mapstructure:"socket_wait_timeout_seconds"` - // TerminateGracePeriodSeconds is the SIGTERM→SIGKILL window when - // force-killing a CH process. Default: 5. - TerminateGracePeriodSeconds int `json:"terminate_grace_period_seconds" mapstructure:"terminate_grace_period_seconds"` - // Log configuration, uses eru core's ServerLogConfig. - Log *coretypes.ServerLogConfig `json:"log" mapstructure:"log"` + // TerminateGracePeriodSeconds: SIGTERM→SIGKILL window when force-killing CH. Default: 5. + TerminateGracePeriodSeconds int `json:"terminate_grace_period_seconds" mapstructure:"terminate_grace_period_seconds"` + Log *coretypes.ServerLogConfig `json:"log" mapstructure:"log"` // Metering selects the lifecycle-event recorder backend. Metering MeteringConfig `json:"metering,omitzero" mapstructure:"metering"` } diff --git a/extend/fs/fs.go b/extend/fs/fs.go index 87b9d7e7..e118f4f8 100644 --- a/extend/fs/fs.go +++ b/extend/fs/fs.go @@ -21,8 +21,7 @@ var ( // (cocoon-fs-) and safe for shell quoting and guest mount commands. validTagRe = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]{0,35}$`) - // ErrUnsupportedBackend signals the resolved hypervisor backend cannot - // hot-plug vhost-user-fs (e.g. Firecracker). + // ErrUnsupportedBackend signals the backend cannot hot-plug vhost-user-fs (e.g. Firecracker). ErrUnsupportedBackend = errors.New("backend does not support fs attach") ) diff --git a/extend/vfio/vfio.go b/extend/vfio/vfio.go index a3b8598a..f20ff6e3 100644 --- a/extend/vfio/vfio.go +++ b/extend/vfio/vfio.go @@ -24,8 +24,7 @@ var ( // "cocoon-" is reserved so cocoon-derived ids never collide. validIDRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]{0,63}$`) - // ErrUnsupportedBackend signals the resolved hypervisor backend cannot - // hot-plug VFIO devices (e.g. Firecracker). + // ErrUnsupportedBackend signals the backend cannot hot-plug VFIO devices (e.g. Firecracker). ErrUnsupportedBackend = errors.New("backend does not support device attach") ) diff --git a/gc/module.go b/gc/module.go index 6c162d0f..b670874b 100644 --- a/gc/module.go +++ b/gc/module.go @@ -21,7 +21,7 @@ type Module[S any] struct { Collect func(ctx context.Context, ids []string, snap S) error } -// Module[S] implements runner — internal to the gc package. +// Module[S] implements runner. func (m Module[S]) getName() string { return m.Name } func (m Module[S]) getLocker() lock.Locker { return m.Locker } diff --git a/gc/refs.go b/gc/refs.go index 5232f9f9..a8a18674 100644 --- a/gc/refs.go +++ b/gc/refs.go @@ -1,6 +1,8 @@ // Cross-module GC protocols: snapshots opt in via the matching method; this file imports nothing concrete. package gc +import "maps" + // usedBlobIDs is implemented by snapshots that reference image blobs. type usedBlobIDs interface { UsedBlobIDs() map[string]struct{} @@ -15,15 +17,12 @@ type activeVMIDs interface { func Collect(others map[string]any, accessor func(any) map[string]struct{}) map[string]struct{} { result := make(map[string]struct{}) for _, snap := range others { - for id := range accessor(snap) { - result[id] = struct{}{} - } + maps.Copy(result, accessor(snap)) } return result } -// BlobIDs extracts blob hex IDs from a snapshot. -// Returns nil if the snapshot does not implement UsedBlobIDs. +// BlobIDs extracts blob hex IDs from a snapshot; nil if it doesn't implement UsedBlobIDs. func BlobIDs(snap any) map[string]struct{} { if u, ok := snap.(usedBlobIDs); ok { return u.UsedBlobIDs() @@ -31,8 +30,7 @@ func BlobIDs(snap any) map[string]struct{} { return nil } -// VMIDs extracts active VM IDs from a snapshot. -// Returns nil if the snapshot does not implement ActiveVMIDs. +// VMIDs extracts active VM IDs from a snapshot; nil if it doesn't implement ActiveVMIDs. func VMIDs(snap any) map[string]struct{} { if a, ok := snap.(activeVMIDs); ok { return a.ActiveVMIDs() diff --git a/gc/runner.go b/gc/runner.go index 95b48503..d17d5830 100644 --- a/gc/runner.go +++ b/gc/runner.go @@ -6,8 +6,7 @@ import ( "github.com/cocoonstack/cocoon/lock" ) -// runner is the internal interface Orchestrator uses to hold heterogeneous -// Module[S] values. Unexported — callers work with Module[S] and Register. +// runner lets Orchestrator hold heterogeneous Module[S] values; callers use Module[S] + Register. type runner interface { getName() string getLocker() lock.Locker diff --git a/hypervisor/cloudhypervisor/api.go b/hypervisor/cloudhypervisor/api.go index 766371cd..ce7767b3 100644 --- a/hypervisor/cloudhypervisor/api.go +++ b/hypervisor/cloudhypervisor/api.go @@ -3,14 +3,14 @@ package cloudhypervisor import "encoding/json" type chVMConfig struct { - // Optional — pointer + omitempty (nil → omitted from JSON). + // Optional — nil omitted from JSON. Payload *chPayload `json:"payload,omitempty"` Balloon *chBalloon `json:"balloon,omitempty"` Serial *chRuntimeFile `json:"serial,omitempty"` Console *chRuntimeFile `json:"console,omitempty"` Vsock *chVsock `json:"vsock,omitempty"` - // Required — value (always present). + // Required. CPUs chCPUs `json:"cpus"` Memory chMemory `json:"memory"` Disks []chDisk `json:"disks,omitempty"` diff --git a/hypervisor/cloudhypervisor/args.go b/hypervisor/cloudhypervisor/args.go index 723d4b37..753dfcc5 100644 --- a/hypervisor/cloudhypervisor/args.go +++ b/hypervisor/cloudhypervisor/args.go @@ -24,7 +24,6 @@ const ( // kvBuilder accumulates key=value CLI fragments. type kvBuilder []string -// String joins all key=value pairs with commas. func (b kvBuilder) String() string { return strings.Join(b, ",") } func (b *kvBuilder) add(kv string) { *b = append(*b, kv) } @@ -97,7 +96,6 @@ func buildVMConfig(_ context.Context, rec *hypervisor.VMRecord, consoleSockPath return cfg } -// buildCLIArgs converts a chVMConfig into cloud-hypervisor CLI arguments. func buildCLIArgs(cfg *chVMConfig, socketPath string) []string { args := []string{"--api-socket", socketPath} @@ -275,7 +273,6 @@ func runtimeFileToCLIArg(c *chRuntimeFile) string { } } -// queueAffinityToCLI converts queue affinity to CH CLI format. func queueAffinityToCLI(qa []chQueueAffinity) string { parts := make([]string, len(qa)) for i, a := range qa { diff --git a/hypervisor/cloudhypervisor/cloudhypervisor.go b/hypervisor/cloudhypervisor/cloudhypervisor.go index e1b31945..fc00a420 100644 --- a/hypervisor/cloudhypervisor/cloudhypervisor.go +++ b/hypervisor/cloudhypervisor/cloudhypervisor.go @@ -11,7 +11,6 @@ import ( const typ = "cloud-hypervisor" -// compile-time interface checks. var ( _ hypervisor.Hypervisor = (*CloudHypervisor)(nil) _ hypervisor.Direct = (*CloudHypervisor)(nil) diff --git a/hypervisor/cloudhypervisor/console.go b/hypervisor/cloudhypervisor/console.go index 00ec2817..47fcf1fe 100644 --- a/hypervisor/cloudhypervisor/console.go +++ b/hypervisor/cloudhypervisor/console.go @@ -22,7 +22,6 @@ func (ch *CloudHypervisor) Console(ctx context.Context, ref string) (io.ReadWrit var conn io.ReadWriteCloser if err := ch.WithRunningVM(ctx, &rec, func(_ int) error { - // Resolve on demand: query CH API for PTY (OCI) or use deterministic socket (UEFI). path := resolveConsole(ctx, id, hypervisor.SocketPath(rec.RunDir), hypervisor.ConsoleSockPath(rec.RunDir), isDirectBoot(rec.BootConfig)) diff --git a/hypervisor/cloudhypervisor/helper.go b/hypervisor/cloudhypervisor/helper.go index e6bb6993..a013e1ef 100644 --- a/hypervisor/cloudhypervisor/helper.go +++ b/hypervisor/cloudhypervisor/helper.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "path/filepath" + "slices" "strings" "time" @@ -85,12 +86,9 @@ func hasMemoryRangeFile(srcDir string) (bool, error) { if err != nil { return false, err } - for _, e := range entries { - if strings.HasPrefix(e.Name(), memoryRangeFile) { - return true, nil - } - } - return false, nil + return slices.ContainsFunc(entries, func(e os.DirEntry) bool { + return strings.HasPrefix(e.Name(), memoryRangeFile) + }), nil } // vmAPIOnce is a single PUT for non-idempotent endpoints; returns raw body so add-fs/add-device can decode PciDeviceInfo. @@ -254,8 +252,7 @@ func queryConsolePTY(ctx context.Context, apiSocketPath string) (string, error) return info.Config.Console.File, nil } -// resolveConsole determines the console path for a VM after launch. -// Direct-boot (OCI) VMs use a PTY allocated by CH; UEFI VMs use a Unix socket. +// resolveConsole returns the CH-allocated PTY (direct-boot OCI) or the console socket (UEFI). func resolveConsole(ctx context.Context, vmID, sockPath, consoleSock string, directBoot bool) string { if directBoot { consolePath, err := utils.DoWithRetry(ctx, func() (string, error) { diff --git a/hypervisor/cloudhypervisor/utils.go b/hypervisor/cloudhypervisor/utils.go index 08ef6c9f..8b69ca0e 100644 --- a/hypervisor/cloudhypervisor/utils.go +++ b/hypervisor/cloudhypervisor/utils.go @@ -23,8 +23,7 @@ func (ch *CloudHypervisor) saveCmdline(ctx context.Context, rec *hypervisor.VMRe } } -// cowPath returns the writable COW disk path for a VM. -// Direct-boot (OCI) uses a raw file; UEFI (cloudimg) uses a qcow2 overlay. +// cowPath returns the writable COW disk: raw for direct-boot (OCI), qcow2 overlay for UEFI (cloudimg). func (ch *CloudHypervisor) cowPath(vmID string, directBoot bool) string { if directBoot { return ch.conf.COWRawPath(vmID) @@ -52,8 +51,7 @@ func qemuExpandImage(ctx context.Context, path string, targetSize int64, directB return nil } -// readQcow2VirtualSize reads the virtual size from a qcow2 file header. -// The qcow2 header stores the virtual size as a big-endian uint64 at offset 24. +// readQcow2VirtualSize reads the big-endian uint64 virtual size at qcow2 header offset 24. func readQcow2VirtualSize(path string) (int64, error) { f, err := os.Open(path) //nolint:gosec if err != nil { diff --git a/hypervisor/firecracker/api.go b/hypervisor/firecracker/api.go index 37536fb0..9806d5af 100644 --- a/hypervisor/firecracker/api.go +++ b/hypervisor/firecracker/api.go @@ -11,7 +11,6 @@ import ( "github.com/cocoonstack/cocoon/utils" ) -// FC REST API constants. const ( actionInstanceStart = "InstanceStart" actionSendCtrlAltDel = "SendCtrlAltDel" @@ -72,7 +71,6 @@ type fcVsock struct { UDSPath string `json:"uds_path"` } -// FC snapshot/load request types. type fcSnapshotCreate struct { SnapshotPath string `json:"snapshot_path"` MemFilePath string `json:"mem_file_path"` diff --git a/hypervisor/firecracker/relay.go b/hypervisor/firecracker/relay.go index 59d61fd3..94b56a0b 100644 --- a/hypervisor/firecracker/relay.go +++ b/hypervisor/firecracker/relay.go @@ -41,8 +41,7 @@ func (b *broadcaster) setSink(w io.Writer) { b.mu.Unlock() } -// readLoop reads from the PTY master forever and writes to the current sink. -// Runs as a single goroutine for the relay's lifetime. +// readLoop runs as the relay's single lifetime PTY-reader goroutine, writing to the current sink. func (b *broadcaster) readLoop() { buf := make([]byte, relayBufSize) for { @@ -105,7 +104,6 @@ func RunRelay(ctx context.Context) { } }() - // Single PTY reader → broadcast to active session so disconnects don't strand goroutines fighting for bytes. bc := &broadcaster{master: master} go bc.readLoop() @@ -126,11 +124,9 @@ func RunRelay(ctx context.Context) { func relaySession(ctx context.Context, master io.Writer, conn net.Conn, bc *broadcaster) { defer conn.Close() //nolint:errcheck - // Subscribe this session to receive PTY output. bc.setSink(conn) defer bc.setSink(nil) - // Copy conn→master (console input) in a goroutine. done := make(chan struct{}) go func() { _, _ = io.Copy(master, conn) @@ -142,5 +138,4 @@ func relaySession(ctx context.Context, master io.Writer, conn net.Conn, bc *broa case <-ctx.Done(): // FC died } // Closing conn unblocks the io.Copy(master, conn) goroutine. - // setSink(nil) in defer stops broadcast to this conn. } diff --git a/hypervisor/firecracker/start.go b/hypervisor/firecracker/start.go index a0d66dd2..6fb06e33 100644 --- a/hypervisor/firecracker/start.go +++ b/hypervisor/firecracker/start.go @@ -156,21 +156,19 @@ func (fc *Firecracker) launchProcess(ctx context.Context, rec *hypervisor.VMReco return 0, err } - // Start console relay as a background process (self-exec). - // The relay holds the PTY master and listens on console.sock. - relayOK := fc.startConsoleRelay(ctx, rec.RunDir, master, pid) == nil - if relayOK { + relayErr := fc.startConsoleRelay(ctx, rec.RunDir, master, pid) + if relayErr == nil { // Master fd ownership transferred to relay; close parent's copy. _ = master.Close() } else { - log.WithFunc("firecracker.launchProcess").Warn(ctx, "console relay failed (console unavailable)") + log.WithFunc("firecracker.launchProcess").Warnf(ctx, "console relay failed (console unavailable): %v", relayErr) } go func() { _ = fcCmd.Wait() // If relay failed, master fd was kept open to preserve ttyS0. // Close it now that FC has exited to avoid permanent fd leak. - if !relayOK { + if relayErr != nil { _ = master.Close() } }() diff --git a/hypervisor/inspect.go b/hypervisor/inspect.go index 0ef52837..a8c3a0e4 100644 --- a/hypervisor/inspect.go +++ b/hypervisor/inspect.go @@ -25,15 +25,11 @@ func (b *Backend) Inspect(ctx context.Context, ref string) (*types.VM, error) { func (b *Backend) List(ctx context.Context) ([]*types.VM, error) { var recs []*VMRecord if err := b.DB.With(ctx, func(idx *VMIndex) error { - recs = make([]*VMRecord, 0, len(idx.VMs)) - for _, r := range idx.VMs { - if r == nil { - continue - } + recs = utils.MapValues(idx.VMs, func(r *VMRecord) *VMRecord { cp := *r cp.SnapshotIDs = maps.Clone(r.SnapshotIDs) - recs = append(recs, &cp) - } + return &cp + }) return nil }); err != nil { return nil, err diff --git a/hypervisor/state_test.go b/hypervisor/state_test.go index c8b5dbc2..401b8aff 100644 --- a/hypervisor/state_test.go +++ b/hypervisor/state_test.go @@ -437,9 +437,6 @@ func TestStartAllOnlyEmitsForActuallyLaunched(t *testing.T) { } entries := rec.Entries() - // vm-stopped → 1 entry (compute.start) - // vm-running → 0 entries (no-op) - // vm-stale → 2 entries (compute.stop reason=stop-crash + compute.start reason=restart) if len(entries) != 3 { t.Fatalf("got %d entries, want 3 (vm-stopped: start; vm-stale: stop-crash + start; vm-running: none)", len(entries)) } diff --git a/hypervisor/utils.go b/hypervisor/utils.go index 54fd2934..6dcb8d88 100644 --- a/hypervisor/utils.go +++ b/hypervisor/utils.go @@ -449,8 +449,7 @@ func EnterNetns(nsPath string) (restore func(), err error) { }, nil } -// createSparseFile creates path as a sparse file truncated to size, matching -// PrepareOCICOW's pattern. os.Truncate alone won't create a missing file. +// createSparseFile creates path truncated to size; os.Truncate alone won't create a missing file. func createSparseFile(path string, size int64) error { f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) //nolint:gosec if err != nil { diff --git a/hypervisor/utils_test.go b/hypervisor/utils_test.go index e9141dae..bd3f43a2 100644 --- a/hypervisor/utils_test.go +++ b/hypervisor/utils_test.go @@ -3,6 +3,7 @@ package hypervisor import ( "os" "path/filepath" + "slices" "strings" "testing" @@ -35,7 +36,7 @@ func TestValidateSnapshotIntegrity(t *testing.T) { }) t.Run("missing data disk", func(t *testing.T) { - bogus := append([]*types.StorageConfig{}, sidecar...) + bogus := slices.Clone(sidecar) bogus = append(bogus, &types.StorageConfig{ Path: "/src/runDir/data-missing.raw", RO: false, Role: types.StorageRoleData, Serial: "missing", FSType: "ext4", diff --git a/images/cloudimg/gc.go b/images/cloudimg/gc.go index a3c3e914..d8ff13a8 100644 --- a/images/cloudimg/gc.go +++ b/images/cloudimg/gc.go @@ -14,7 +14,7 @@ func (c *CloudImg) GCModule() gc.Module[images.ImageGCSnapshot] { Locker: c.locker, Store: c.store, ReadRefs: func(idx *imageIndex) map[string]struct{} { return images.ReferencedDigests(idx.Images) }, - ScanDisk: func() ([]string, error) { return utils.ScanFileStems(c.conf.BlobsDir(), ".qcow2") }, + ScanDisk: func() ([]string, error) { return utils.ScanFileStems(c.conf.BlobsDir(), c.conf.BlobExt) }, Removers: []func(string) error{ func(hex string) error { return os.Remove(c.conf.BlobPath(hex)) }, }, diff --git a/images/cloudimg/inspect.go b/images/cloudimg/inspect.go index 6adebb05..0e728904 100644 --- a/images/cloudimg/inspect.go +++ b/images/cloudimg/inspect.go @@ -14,28 +14,23 @@ import ( "github.com/cocoonstack/cocoon/utils" ) -var ( - // qcow2Magic is the qcow2 file signature. - qcow2Magic = []byte{'Q', 'F', 'I', 0xfb} +// nonImageSignatures catches common payloads qemu-img would misclassify as raw. +var nonImageSignatures = []struct { + prefix []byte + desc string +}{ + {[]byte("-". -// Returns the prefix and true, or ("", false) if the name doesn't match. func parseTAPName(name string) (string, bool) { rest, ok := strings.CutPrefix(name, tapPrefix) if !ok { diff --git a/network/cni/cni.go b/network/cni/cni.go index cf3ee269..855d37de 100644 --- a/network/cni/cni.go +++ b/network/cni/cni.go @@ -36,6 +36,7 @@ type CNI struct { confLists map[string]*libcni.NetworkConfigList // name → conflist defaultName string // first conflist name (backward compat) cniConf *libcni.CNIConfig + loadErr error // conflist load failure, surfaced by errNoConflist } // New creates a CNI provider; conflist loading is best-effort so Delete/Inspect/List still work when none are available — Add fails in that case. @@ -66,6 +67,8 @@ func New(conf *config.Config) (*CNI, error) { cfg.CacheDir(), nil, ) + } else { + c.loadErr = loadErr } return c, nil @@ -83,8 +86,7 @@ func (c *CNI) Verify(_ context.Context, vmID string) error { return nil } -// Inspect returns the network record for a single network ID. -// Returns (nil, nil) if not found. +// Inspect returns the network record for id, or (nil, nil) if not found. func (c *CNI) Inspect(ctx context.Context, id string) (*types.Network, error) { var result *types.Network return result, c.store.With(ctx, func(idx *networkIndex) error { @@ -92,7 +94,7 @@ func (c *CNI) Inspect(ctx context.Context, id string) (*types.Network, error) { if rec == nil { return nil } - net := rec.Network // value copy + net := rec.Network // value copy — detached from the locked index result = &net return nil }) @@ -194,8 +196,7 @@ func (c *CNI) deleteRecords(ctx context.Context, ids []string) error { }) } -// confListByName resolves a conflist by name. -// Empty name returns the default (first alphabetically). +// confListByName resolves a conflist by name; empty name returns the default (first alphabetically). func (c *CNI) confListByName(name string) (*libcni.NetworkConfigList, error) { if len(c.confLists) == 0 { return nil, c.errNoConflist() @@ -208,11 +209,13 @@ func (c *CNI) confListByName(name string) (*libcni.NetworkConfigList, error) { } func (c *CNI) errNoConflist() error { + if c.loadErr != nil { + return fmt.Errorf("%w: load conflists from %s: %w", network.ErrNotConfigured, c.conf.CNIConfDir, c.loadErr) + } return fmt.Errorf("%w: no conflist found in %s", network.ErrNotConfigured, c.conf.CNIConfDir) } -// loadConfLists loads all .conflist files from dir. -// Returns the map of name→conflist and the default name (first file, alphabetically). +// loadConfLists loads all .conflist files from dir, returning name→conflist and the default (alphabetically first) name. func loadConfLists(dir string) (map[string]*libcni.NetworkConfigList, string, error) { files, err := libcni.ConfFiles(dir, []string{".conflist"}) if err != nil { diff --git a/network/cni/db.go b/network/cni/db.go index 183a7ad8..6c57e35a 100644 --- a/network/cni/db.go +++ b/network/cni/db.go @@ -8,11 +8,9 @@ import ( // Keyed by a generated network ID (unique per NIC, not per VM). type networkRecord struct { types.Network `json:"network"` - // ID is the unique network record identifier (map key in networkIndex). - ID string `json:"id"` + ID string `json:"id"` // Type is the CNI conflist name (e.g. "cocoon", "calico"). Type string `json:"type"` - // VMID links this network back to the owning VM. VMID string `json:"vm_id"` // IfName is the CNI interface name inside the netns (eth0, eth1, ...). IfName string `json:"if_name"` @@ -30,8 +28,7 @@ func (idx *networkIndex) Init() { } } -// byVMID returns copies of all network records belonging to vmID. -// Returns detached copies safe to use after the lock is released. +// byVMID returns detached copies of vmID's records, safe to use after the lock is released. func (idx *networkIndex) byVMID(vmID string) []networkRecord { var out []networkRecord for _, rec := range idx.Networks { diff --git a/network/cni/gc.go b/network/cni/gc.go index 89095455..d808584d 100644 --- a/network/cni/gc.go +++ b/network/cni/gc.go @@ -38,8 +38,6 @@ func (c *CNI) GCModule() gc.Module[cniSnapshot] { }); err != nil { return snap, err } - // Scan named netns with the cocoon- prefix only. - // Other tools (docker, containerd) may have their own entries. if entries, readErr := os.ReadDir(netnsBasePath); readErr == nil { for _, e := range entries { if name, ok := strings.CutPrefix(e.Name(), netnsPrefix); ok { @@ -81,7 +79,7 @@ func (c *CNI) GCModule() gc.Module[cniSnapshot] { // 2. CNI DEL per NIC — best-effort IPAM release. _ = c.tearDownNICs(ctx, vmID, netnsPath(vmID), records, false, true) - // 3. Remove the named netns (with retry for async kernel fd cleanup). + // 3. Remove the named netns. nsName := netnsName(vmID) if err := deleteNetns(ctx, nsName); err != nil && !errors.Is(err, fs.ErrNotExist) { errs = append(errs, fmt.Errorf("remove netns %s: %w", nsName, err)) diff --git a/network/network.go b/network/network.go index 9abf7f0f..2d6b9bf5 100644 --- a/network/network.go +++ b/network/network.go @@ -8,10 +8,7 @@ import ( "github.com/cocoonstack/cocoon/types" ) -var ( - ErrNotFound = errors.New("network not found") - ErrNotConfigured = errors.New("network provider not configured") -) +var ErrNotConfigured = errors.New("network provider not configured") // AddSpec is one NIC's add request; Existing != nil reuses MAC/IP for recovery. type AddSpec struct { diff --git a/progress/progress.go b/progress/progress.go index cb7bd4a8..805e45af 100644 --- a/progress/progress.go +++ b/progress/progress.go @@ -3,17 +3,11 @@ package progress // Nop is a no-op tracker for callers that don't need progress reporting. var Nop Tracker = funcTracker(func(any) {}) -// Tracker receives progress events during image operations. -// Implementations must be safe for concurrent use from multiple goroutines. +// Tracker receives progress events during image operations; implementations must be safe for concurrent use. type Tracker interface { OnEvent(any) } -type funcTracker func(any) - -// OnEvent dispatches a progress event to the wrapped callback. -func (f funcTracker) OnEvent(e any) { f(e) } - // NewTracker wraps a typed callback as a non-generic Tracker so Images can hold it in its interface. func NewTracker[E any](fn func(E)) Tracker { return funcTracker(func(v any) { @@ -22,3 +16,7 @@ func NewTracker[E any](fn func(E)) Tracker { } }) } + +type funcTracker func(any) + +func (f funcTracker) OnEvent(e any) { f(e) } diff --git a/snapshot/localfile/export.go b/snapshot/localfile/export.go index 10be87ef..d24b8f2b 100644 --- a/snapshot/localfile/export.go +++ b/snapshot/localfile/export.go @@ -33,8 +33,7 @@ func (lf *LocalFile) ExportToDir(ctx context.Context, ref, dir string) error { if err = utils.EnsureDirs(dir); err != nil { return err } - // Reject non-empty targets so the export can't silently merge into an - // unrelated tree. + // Reject non-empty targets so the export can't merge into an unrelated tree. dstEntries, err := os.ReadDir(dir) if err != nil { return fmt.Errorf("read %s: %w", dir, err) diff --git a/snapshot/localfile/gc.go b/snapshot/localfile/gc.go index 55af97c3..8bd9fa89 100644 --- a/snapshot/localfile/gc.go +++ b/snapshot/localfile/gc.go @@ -150,7 +150,7 @@ func gcModule(conf *Config, store storage.Store[snapshot.SnapshotIndex], locker } } -// pickLRU returns evict IDs keyed by reason ("+" joins multi-match; no criteria → "lru-all"). +// pickLRU maps each evict ID to its reason ("+" joins multi-match; no criteria → "lru-all"). func pickLRU(records map[string]snapshotMeta, p EvictionPolicy) map[string]string { sorted := slices.SortedFunc(maps.Keys(records), func(a, b string) int { return records[a].lastAccessed.Compare(records[b].lastAccessed) diff --git a/snapshot/localfile/import.go b/snapshot/localfile/import.go index 2fb6f7e0..e7b6ce8d 100644 --- a/snapshot/localfile/import.go +++ b/snapshot/localfile/import.go @@ -1,6 +1,7 @@ package localfile import ( + "bytes" "cmp" "compress/gzip" "context" @@ -76,8 +77,7 @@ func (lf *LocalFile) Import(ctx context.Context, r io.Reader, name, description return id, nil } -// unwrapGzip peeks at the first 2 bytes to detect gzip magic (0x1f 0x8b). -// Returns the underlying tar reader and an optional gzip closer. +// unwrapGzip sniffs the 2-byte gzip magic (0x1f 0x8b) and returns the tar reader plus a gzip closer (nil for raw tar). func unwrapGzip(r io.Reader) (io.Reader, io.Closer, error) { head, full, err := utils.PeekReader(r, 2) if err != nil { @@ -86,7 +86,7 @@ func unwrapGzip(r io.Reader) (io.Reader, io.Closer, error) { if len(head) < 2 { return nil, nil, errors.New("peek archive header: stream shorter than gzip magic (2 bytes)") } - if head[0] == 0x1f && head[1] == 0x8b { + if bytes.HasPrefix(head, utils.GzipMagic) { gr, gzErr := gzip.NewReader(full) if gzErr != nil { return nil, nil, fmt.Errorf("decompress: %w", gzErr) diff --git a/snapshot/localfile/localfile.go b/snapshot/localfile/localfile.go index 5462d3df..afd86330 100644 --- a/snapshot/localfile/localfile.go +++ b/snapshot/localfile/localfile.go @@ -24,7 +24,6 @@ import ( const typ = "localfile" -// compile-time interface checks. var ( _ snapshot.Snapshot = (*LocalFile)(nil) _ snapshot.Direct = (*LocalFile)(nil) @@ -40,6 +39,7 @@ func WithGCPolicy(p EvictionPolicy) Option { return func(lf *LocalFile) { lf.gcPolicy = p } } +// LocalFile is the local-filesystem snapshot backend. type LocalFile struct { conf *Config store storage.Store[snapshot.SnapshotIndex] diff --git a/snapshot/localfile/localfile_test.go b/snapshot/localfile/localfile_test.go index 6adc48c7..aeee58be 100644 --- a/snapshot/localfile/localfile_test.go +++ b/snapshot/localfile/localfile_test.go @@ -151,8 +151,7 @@ func TestDeleteOneIdempotentDoesNotEmitTwice(t *testing.T) { t.Fatalf("second deleteOne (idempotent): %v", err) } - // Ledger should hold exactly 2 entries: Create's start and the FIRST - // deleteOne's stop. The second call must not contribute a phantom event. + // Exactly 2 entries: Create's start + the first deleteOne's stop. entries := rec.Entries() if len(entries) != 2 { t.Fatalf("got %d entries, want 2 (start + 1× stop); kinds = %v", len(entries), kinds(entries)) @@ -825,7 +824,6 @@ func TestRestore_DoubleCloseNoPanic(t *testing.T) { t.Fatal(err) } - // First close — should work normally. rc.Close() // Second close — must not deadlock or panic (idempotent via sync.Once). rc.Close() @@ -1012,7 +1010,6 @@ func TestImport_FromGzipTarReader(t *testing.T) { gw := gzip.NewWriter(&buf) tw := tar.NewWriter(gw) - // snapshot.json entry. if err := tw.WriteHeader(&tar.Header{ Name: "snapshot.json", Size: int64(len(jsonData)), Mode: 0o644, Typeflag: tar.TypeReg, }); err != nil { @@ -1022,7 +1019,6 @@ func TestImport_FromGzipTarReader(t *testing.T) { t.Fatal(err) } - // data file entry. dataContent := []byte("state data") if err := tw.WriteHeader(&tar.Header{ Name: "state.json", Size: int64(len(dataContent)), Mode: 0o644, Typeflag: tar.TypeReg, diff --git a/snapshot/snapshot.go b/snapshot/snapshot.go index afab0a9d..b9dd4fbd 100644 --- a/snapshot/snapshot.go +++ b/snapshot/snapshot.go @@ -38,8 +38,7 @@ type Snapshot interface { // Restore restores a snapshot by ID or name, returning the snapshot config and a data stream. Restore(ctx context.Context, ref string) (types.SnapshotConfig, io.ReadCloser, error) - // Export streams the snapshot as a raw tar archive. - // The archive includes a snapshot.json metadata entry followed by data files. + // Export streams the snapshot as a raw tar (snapshot.json entry first, then data files). Export(ctx context.Context, ref string) (io.ReadCloser, error) // Import reads a snapshot tar (gzip auto-detected); non-empty name/description override the envelope. Import(ctx context.Context, r io.Reader, name, description string) (string, error) diff --git a/storage/json/json_test.go b/storage/json/json_test.go index d5710166..0e83d3f0 100644 --- a/storage/json/json_test.go +++ b/storage/json/json_test.go @@ -32,7 +32,6 @@ func TestLoadFreshFromDisk(t *testing.T) { dir := t.TempDir() dataPath := filepath.Join(dir, "data.json") - // Write directly to disk. original := testData{Name: "alice", Count: 1} raw, _ := json.Marshal(original) if err := os.WriteFile(dataPath, raw, 0o644); err != nil { @@ -41,7 +40,6 @@ func TestLoadFreshFromDisk(t *testing.T) { s := New[testData](dataPath, flock.New(filepath.Join(dir, "data.lock"))) - // First read. if err := s.ReadRaw(func(d *testData) error { if d.Name != "alice" { t.Fatalf("expected alice, got %s", d.Name) @@ -80,7 +78,6 @@ func TestCrossInstanceVisibility(t *testing.T) { a := newTestStore(t, dir, "shared") b := newTestStore(t, dir, "shared") - // A writes. if err := a.Update(ctx, func(d *testData) error { d.Name = "from-a" d.Count = 42 @@ -110,7 +107,6 @@ func TestTryLockThenReadRaw(t *testing.T) { a := newTestStore(t, dir, "trylock") b := newTestStore(t, dir, "trylock") - // A writes initial data. if err := a.Update(ctx, func(d *testData) error { d.Name = "initial" return nil @@ -118,7 +114,6 @@ func TestTryLockThenReadRaw(t *testing.T) { t.Fatal(err) } - // B acquires via TryLock, uses WriteRaw, then Unlock. ok, err := b.TryLock(ctx) if err != nil { t.Fatal(err) diff --git a/storage/storage.go b/storage/storage.go index 30dd7c1f..352ca523 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -9,17 +9,14 @@ type Initer interface { Init() } -// Store provides locked read/modify/write access to a data store. -// T is the top-level structure managed by the store. +// Store provides locked read/modify/write access to a data store of top-level type T. type Store[T any] interface { // With loads under lock and passes to fn; *T's Init() runs first if implemented; lock held for fn's duration. With(ctx context.Context, fn func(*T) error) error - // Update performs a read-modify-write under lock. - // If fn returns nil the data is persisted. + // Update performs a read-modify-write under lock; persists only if fn returns nil. Update(ctx context.Context, fn func(*T) error) error - // ReadRaw deserializes the data and passes it to fn without acquiring the lock. - // The caller must already hold the lock via TryLock. + // ReadRaw deserializes and passes to fn without locking; caller must already hold the lock via TryLock. ReadRaw(fn func(*T) error) error // WriteRaw deserializes, runs fn, atomically persists; caller must hold the lock (via TryLock). WriteRaw(fn func(*T) error) error diff --git a/types/boot.go b/types/boot.go index cef0d8f1..398a80b5 100644 --- a/types/boot.go +++ b/types/boot.go @@ -5,8 +5,7 @@ type BootConfig struct { // Direct-boot fields (OCI images). KernelPath string `json:"kernel_path,omitempty"` InitrdPath string `json:"initrd_path,omitempty"` - // Cmdline is the kernel command line for direct-boot VMs. - // Set at Create time from the storage layout (cocoon.layers=, cocoon.cow=, …). + // Cmdline is the direct-boot kernel command line, set at Create from the storage layout (cocoon.layers=, cocoon.cow=, …). Cmdline string `json:"cmdline,omitempty"` // UEFI-boot field (cloud images). diff --git a/types/network.go b/types/network.go index 2bdf0d22..482559db 100644 --- a/types/network.go +++ b/types/network.go @@ -13,8 +13,7 @@ type NetworkConfig struct { NumQueues int `json:"num_queues"` // Virtio queue count (= CPU * 2 for multi-queue). QueueSize int `json:"queue_size"` - // Backend is the provider type ("cni" or "bridge"); empty means "cni" for - // backward compat with pre-bridge VM records. + // Backend is the provider type ("cni" or "bridge"); empty means "cni" (pre-bridge records). Backend string `json:"backend,omitempty"` // BridgeDev is the Linux bridge device name; set only when Backend=="bridge". diff --git a/types/storage.go b/types/storage.go index ae824341..1f3f1692 100644 --- a/types/storage.go +++ b/types/storage.go @@ -13,7 +13,7 @@ const ( StorageRoleCidata StorageRole = "cidata" StorageRoleData StorageRole = "data" - // Phase 1 fstype values for Role==Data disks. + // fstype values for Role==Data disks. FSTypeExt4 = "ext4" FSTypeNone = "none" ) @@ -21,8 +21,7 @@ const ( // dataDiskNameRe caps length at 20 to match Linux's /dev/disk/by-id/virtio- truncation. var dataDiskNameRe = regexp.MustCompile(`^[a-z][a-z0-9_-]{0,19}$`) -// StorageRole classifies a disk's purpose in the VM. Required on every -// StorageConfig — empty values are rejected by ValidateStorageConfigs. +// StorageRole classifies a disk's purpose; empty is rejected by ValidateStorageConfigs. type StorageRole string // StorageConfig describes a disk attached to a VM. @@ -91,8 +90,7 @@ func ValidateStorageConfigs(configs []*StorageConfig) error { return nil } -// ValidDataDiskName reports whether s is a legal data disk name. -// Shared between CLI parsing and sidecar loading (sidecar may be untrusted). +// ValidDataDiskName reports whether s is a legal data disk name; shared with untrusted sidecar loading. func ValidDataDiskName(s string) bool { if !dataDiskNameRe.MatchString(s) { return false diff --git a/types/vm.go b/types/vm.go index 261f1cf5..49b1585b 100644 --- a/types/vm.go +++ b/types/vm.go @@ -36,8 +36,7 @@ type VMConfig struct { DataDisks []DataDiskSpec `json:"-"` // populated from --data-disk; consumed by Create } -// NetSetup is the VM's host networking state: backend, netns, bridge, and attached NICs. -// Embedded into VM and also used as the initNetwork → hypervisor handoff. +// 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"` NetnsPath string `json:"netns_path,omitempty"` @@ -57,17 +56,15 @@ type VM struct { SocketPath string `json:"socket_path,omitempty"` // CH API Unix socket VsockSocket string `json:"vsock_socket,omitempty"` // hybrid vsock UDS for cocoon-agent - // Network — embedded; fields promote (vm.NetBackend, vm.NetworkConfigs, ...). + // Network. NetSetup StorageConfigs []*StorageConfig `json:"storage_configs,omitempty"` - // FirstBooted is true after the VM has been started at least once. - // Used to skip cidata attachment on subsequent starts (cloudimg only). + // FirstBooted is set after the first start; skips cidata attachment on later starts (cloudimg only). FirstBooted bool `json:"first_booted"` - // SnapshotIDs tracks snapshots created from this VM. - // Populated at runtime by toVM() from VMRecord.SnapshotIDs. + // SnapshotIDs tracks snapshots created from this VM; populated by toVM() from VMRecord.SnapshotIDs. SnapshotIDs map[string]struct{} `json:"snapshot_ids,omitempty"` // Timestamps. @@ -117,8 +114,8 @@ func (v *VM) ResolvedNetnsPath() string { if v.NetnsPath != "" { return v.NetnsPath } - if len(v.NetworkConfigs) > 0 { - return v.NetworkConfigs[0].NetnsPath + if nic := v.firstNIC(); nic != nil { + return nic.NetnsPath } return "" } @@ -131,9 +128,9 @@ func (v *VM) ResolvedNetBackend() string { if v.NetBackend != "" { return v.NetBackend } - if len(v.NetworkConfigs) > 0 { - if b := v.NetworkConfigs[0].Backend; b != "" { - return b + if nic := v.firstNIC(); nic != nil { + if nic.Backend != "" { + return nic.Backend } return BackendCNI } @@ -148,8 +145,16 @@ func (v *VM) ResolvedNetBridgeDev() string { if v.NetBridgeDev != "" { return v.NetBridgeDev } - if len(v.NetworkConfigs) > 0 { - return v.NetworkConfigs[0].BridgeDev + if nic := v.firstNIC(); nic != nil { + return nic.BridgeDev } return "" } + +// firstNIC returns NIC[0] (nil when absent or the entry is null). +func (v *VM) firstNIC() *NetworkConfig { + if v == nil || len(v.NetworkConfigs) == 0 { + return nil + } + return v.NetworkConfigs[0] +} diff --git a/utils/atomic.go b/utils/atomic.go index 6434782d..82857a26 100644 --- a/utils/atomic.go +++ b/utils/atomic.go @@ -9,8 +9,7 @@ import ( "syscall" ) -// AtomicWriteFile writes data to a file atomically using temp + fsync + rename. -// This prevents partial writes from being visible to readers. +// AtomicWriteFile writes data via temp + fsync + rename so readers never see a partial file. func AtomicWriteFile(path string, data []byte, perm os.FileMode) error { dir := filepath.Dir(path) tmp, err := os.CreateTemp(dir, ".tmp-*") diff --git a/utils/atomic_test.go b/utils/atomic_test.go index f021c963..37c12a04 100644 --- a/utils/atomic_test.go +++ b/utils/atomic_test.go @@ -118,7 +118,6 @@ func TestAtomicWriteJSON_Basic(t *testing.T) { t.Errorf("got %v", got) } - // Should end with newline. if data[len(data)-1] != '\n' { t.Error("expected trailing newline") } diff --git a/utils/file.go b/utils/file.go index 12fd9525..dd1c7a1d 100644 --- a/utils/file.go +++ b/utils/file.go @@ -107,8 +107,7 @@ func FilterUnreferenced(candidates []string, refs map[string]struct{}, exclude . return out } -// RemoveMatching scans dir and removes entries where match returns true. -// Returns a slice of errors for entries that could not be removed. +// RemoveMatching removes entries in dir where match returns true, returning per-entry removal errors. func RemoveMatching(ctx context.Context, dir string, match func(os.DirEntry) bool) []error { entries, err := os.ReadDir(dir) if err != nil { diff --git a/utils/file_test.go b/utils/file_test.go index 303c6fc0..6944714b 100644 --- a/utils/file_test.go +++ b/utils/file_test.go @@ -3,7 +3,7 @@ package utils import ( "os" "path/filepath" - "sort" + "slices" "testing" ) @@ -32,7 +32,6 @@ func TestEnsureDirs_CreateNew(t *testing.T) { func TestEnsureDirs_AlreadyExist(t *testing.T) { dir := t.TempDir() - // Should not error when dirs already exist. if err := EnsureDirs(dir); err != nil { t.Fatalf("EnsureDirs on existing dir: %v", err) } @@ -49,7 +48,6 @@ func TestEnsureDirs_FailsUnderFile(t *testing.T) { file := filepath.Join(dir, "regular_file") os.WriteFile(file, []byte("x"), 0o644) //nolint:errcheck - // Trying to create a dir under a regular file should fail. err := EnsureDirs(filepath.Join(file, "subdir")) if err == nil { t.Fatal("expected error when creating dir under a file") @@ -104,7 +102,7 @@ func TestScanFileStems_Basic(t *testing.T) { if err != nil { t.Fatal(err) } - sort.Strings(stems) + slices.Sort(stems) if len(stems) != 2 || stems[0] != "abc" || stems[1] != "def" { t.Errorf("got %v, want [abc def]", stems) } @@ -154,7 +152,7 @@ func TestScanSubdirs_Basic(t *testing.T) { if err != nil { t.Fatal(err) } - sort.Strings(subs) + slices.Sort(subs) if len(subs) != 2 || subs[0] != "sub1" || subs[1] != "sub2" { t.Errorf("got %v, want [sub1 sub2]", subs) } @@ -198,7 +196,7 @@ func TestFilterUnreferenced_Basic(t *testing.T) { refs := map[string]struct{}{"a": {}, "c": {}} got := FilterUnreferenced(candidates, refs) - sort.Strings(got) + slices.Sort(got) if len(got) != 2 || got[0] != "b" || got[1] != "d" { t.Errorf("got %v, want [b d]", got) } @@ -210,7 +208,7 @@ func TestFilterUnreferenced_WithExclude(t *testing.T) { exclude := map[string]struct{}{"b": {}} got := FilterUnreferenced(candidates, refs, exclude) - sort.Strings(got) + slices.Sort(got) if len(got) != 2 || got[0] != "c" || got[1] != "d" { t.Errorf("got %v, want [c d]", got) } @@ -250,7 +248,7 @@ func TestFilterUnreferenced_MultipleExcludeSets(t *testing.T) { ex2 := map[string]struct{}{"c": {}} got := FilterUnreferenced(candidates, refs, ex1, ex2) - sort.Strings(got) + slices.Sort(got) if len(got) != 2 || got[0] != "d" || got[1] != "e" { t.Errorf("got %v, want [d e]", got) } diff --git a/utils/http_test.go b/utils/http_test.go index 6d9d14e1..32464144 100644 --- a/utils/http_test.go +++ b/utils/http_test.go @@ -192,7 +192,7 @@ func TestDoAPI_ContextCanceled(t *testing.T) { defer srv.Close() ctx, cancel := context.WithCancel(t.Context()) - cancel() // cancel immediately + cancel() _, err := DoAPI(ctx, srv.Client(), http.MethodGet, srv.URL+"/slow", nil, http.StatusOK) if err == nil { @@ -279,7 +279,6 @@ func TestDoWithRetry_ExhaustedRetries(t *testing.T) { if err == nil { t.Fatal("expected error after exhausted retries") } - // MaxRetries=3, so total attempts = MaxRetries+1 = 4 if calls != MaxRetries+1 { t.Errorf("expected %d calls, got %d", MaxRetries+1, calls) } diff --git a/utils/hugepages.go b/utils/hugepages.go index e432b03b..283a9823 100644 --- a/utils/hugepages.go +++ b/utils/hugepages.go @@ -8,7 +8,7 @@ import ( "strings" ) -// DetectHugePages returns true iff /proc/sys/vm/nr_hugepages > 0; false on any error (non-Linux, etc.). +// DetectHugePages returns true iff /proc/sys/vm/nr_hugepages > 0; false on any read/parse error. func DetectHugePages() bool { data, err := os.ReadFile("/proc/sys/vm/nr_hugepages") if err != nil { diff --git a/utils/magic.go b/utils/magic.go new file mode 100644 index 00000000..8deb5f69 --- /dev/null +++ b/utils/magic.go @@ -0,0 +1,10 @@ +package utils + +// Magic byte prefixes shared by image/blob format sniffing. +var ( + // Qcow2Magic is the qcow2 file signature ("QFI\xfb"). + Qcow2Magic = []byte{'Q', 'F', 'I', 0xfb} + + // GzipMagic is the gzip stream signature. + GzipMagic = []byte{0x1f, 0x8b} +) diff --git a/utils/process_test.go b/utils/process_test.go index 851f9096..7be4c1e8 100644 --- a/utils/process_test.go +++ b/utils/process_test.go @@ -83,7 +83,6 @@ func TestWriteReadPIDFile_Roundtrip_LargePID(t *testing.T) { func TestReadPIDFile_WhitespaceHandling(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "ws.pid") - // Extra whitespace around PID. if err := os.WriteFile(path, []byte(" 42 \n"), 0o600); err != nil { t.Fatal(err) } @@ -121,7 +120,6 @@ func TestIsProcessAlive_InvalidPID(t *testing.T) { } func TestIsProcessAlive_DeadProcess(t *testing.T) { - // Start a process and wait for it to exit, then check. cmd := exec.Command("true") if err := cmd.Start(); err != nil { t.Fatalf("start: %v", err) @@ -129,9 +127,6 @@ func TestIsProcessAlive_DeadProcess(t *testing.T) { pid := cmd.Process.Pid _ = cmd.Wait() - // Process has exited; PID may be recycled eventually, - // but immediately after Wait it should not be alive. - // Allow a small race window by retrying. if IsProcessAlive(pid) { // PID recycled extremely fast — skip rather than fail. t.Skip("PID recycled too quickly, skipping") @@ -151,13 +146,10 @@ func TestVerifyProcessCmdline_WrongBinary(t *testing.T) { pid := os.Getpid() // On Linux, this checks /proc/pid/cmdline; on other platforms falls back to IsProcessAlive. result := VerifyProcessCmdline(pid, "definitely-not-the-binary", "definitely-not-the-arg") - // On Linux, should return false (cmdline doesn't contain these strings). - // On other platforms, falls back to IsProcessAlive (true). _ = result // Just verify no panic. } func TestTerminateProcess_SleepProcess(t *testing.T) { - // Start a sleep process we can terminate. cmd := exec.Command("sleep", "60") if err := cmd.Start(); err != nil { t.Fatalf("start sleep: %v", err) @@ -191,7 +183,6 @@ func TestTerminateProcess_SleepProcess(t *testing.T) { } func TestTerminateProcess_AlreadyDead(t *testing.T) { - // Start and immediately wait. cmd := exec.Command("true") if err := cmd.Start(); err != nil { t.Fatal(err) @@ -287,7 +278,6 @@ func TestFindVMMByCmdline(t *testing.T) { } func TestTerminateProcess_ContextCancelled(t *testing.T) { - // Start a process that ignores SIGTERM (sleep handles it by default though). cmd := exec.Command("sleep", "60") if err := cmd.Start(); err != nil { t.Fatal(err) @@ -299,9 +289,8 @@ func TestTerminateProcess_ContextCancelled(t *testing.T) { }() ctx, cancel := context.WithCancel(t.Context()) - cancel() // Cancel immediately. + cancel() - // With cancelled context, TerminateProcess should still attempt to kill. - // It may return context error from WaitFor, but the process should be killed. + // Cancelled ctx: may return WaitFor's ctx error but must still attempt the kill. _ = TerminateProcess(ctx, pid, "sleep", "60", 100*time.Millisecond) } diff --git a/utils/resolve.go b/utils/resolve.go index a6b79dc5..d4482a13 100644 --- a/utils/resolve.go +++ b/utils/resolve.go @@ -6,7 +6,6 @@ import ( ) // ResolveRef resolves a ref (exact ID, name, or ID prefix ≥3 chars) to a full ID. -// Works with any Index that has an Items map and a Names map. func ResolveRef[T any](items map[string]*T, names map[string]string, ref string, notFound error) (string, error) { if items[ref] != nil { return ref, nil @@ -16,8 +15,8 @@ func ResolveRef[T any](items map[string]*T, names map[string]string, ref string, } if len(ref) >= 3 { var match string - for id := range items { - if strings.HasPrefix(id, ref) { + for id, item := range items { + if item != nil && strings.HasPrefix(id, ref) { if match != "" { return "", fmt.Errorf("ambiguous ref %q: multiple matches", ref) } diff --git a/utils/sparse_linux_test.go b/utils/sparse_linux_test.go index fc8ff9a6..9c4fb2cb 100644 --- a/utils/sparse_linux_test.go +++ b/utils/sparse_linux_test.go @@ -10,8 +10,6 @@ import ( "testing" ) -// helpers - // writeAt writes data at the given offset in f, leaving a hole before it if offset > current size. func writeAt(t *testing.T, f *os.File, offset int64, data []byte) { t.Helper() @@ -44,8 +42,6 @@ func readFull(t *testing.T, path string) []byte { return data } -// tests - func TestSparseCopy_AllSparse(t *testing.T) { dir := t.TempDir() src := filepath.Join(dir, "src") @@ -66,7 +62,6 @@ func TestSparseCopy_AllSparse(t *testing.T) { t.Fatalf("SparseCopy: %v", err) } - // Verify size matches. dstInfo, err := os.Stat(dst) if err != nil { t.Fatal(err) @@ -75,13 +70,11 @@ func TestSparseCopy_AllSparse(t *testing.T) { t.Errorf("size: got %d, want %d", dstInfo.Size(), size) } - // Verify content is all zeros. got := readFull(t, dst) if !bytes.Equal(got, make([]byte, size)) { t.Error("content mismatch: expected all zeros") } - // Verify dst is sparse (very few blocks allocated). blocks := fileBlocks(t, dst) if blocks > 16 { // 16 * 512 = 8KB — generous threshold for metadata t.Errorf("expected sparse dst, got %d blocks", blocks) @@ -102,7 +95,6 @@ func TestSparseCopy_PartialData(t *testing.T) { t.Fatal(err) } - // Write a small chunk at offset 4096. data := []byte("hello sparse world!") writeAt(t, f, 4096, data) f.Close() @@ -111,14 +103,12 @@ func TestSparseCopy_PartialData(t *testing.T) { t.Fatalf("SparseCopy: %v", err) } - // Content must match exactly. srcData := readFull(t, src) dstData := readFull(t, dst) if !bytes.Equal(srcData, dstData) { t.Error("content mismatch") } - // Dst should be sparse — far fewer blocks than full size. blocks := fileBlocks(t, dst) fullBlocks := int64(size / 512) if blocks >= fullBlocks/2 { @@ -131,7 +121,6 @@ func TestSparseCopy_NonSparse(t *testing.T) { src := filepath.Join(dir, "src") dst := filepath.Join(dir, "dst") - // Create a fully written file. data := bytes.Repeat([]byte("X"), 64*1024) // 64KB if err := os.WriteFile(src, data, 0o644); err != nil { t.Fatal(err) @@ -209,7 +198,6 @@ func TestSparseCopy_MultiSegment(t *testing.T) { t.Error("content mismatch") } - // Should be sparse. blocks := fileBlocks(t, dst) fullBlocks := int64(size / 512) if blocks >= fullBlocks/2 { diff --git a/utils/sparse_test.go b/utils/sparse_test.go index 7b3e75ca..d3c3fda8 100644 --- a/utils/sparse_test.go +++ b/utils/sparse_test.go @@ -7,9 +7,7 @@ import ( "testing" ) -// These tests exercise SparseCopy on all platforms. -// On Linux, SparseCopy preserves sparsity via SEEK_DATA/SEEK_HOLE. -// On other platforms, SparseCopy falls back to a plain io.Copy. +// Cross-platform SparseCopy tests; Linux-only sparsity assertions live in sparse_linux_test.go. func TestSparseCopy_BasicContent(t *testing.T) { dir := t.TempDir() @@ -95,7 +93,6 @@ func TestSparseCopy_DstDirNotExist(t *testing.T) { t.Fatal(err) } - // Destination in a directory that doesn't exist. err := SparseCopy(filepath.Join(dir, "nodir", "dst"), src) if err == nil { t.Fatal("expected error for nonexistent dst directory") @@ -107,7 +104,6 @@ func TestSparseCopy_OverwritesExisting(t *testing.T) { src := filepath.Join(dir, "src") dst := filepath.Join(dir, "dst") - // Write initial dst content. if err := os.WriteFile(dst, []byte("old content that should be overwritten"), 0o644); err != nil { t.Fatal(err) } diff --git a/utils/tar_sparse_linux.go b/utils/tar_sparse_linux.go index 2579f0f8..a3b61e5a 100644 --- a/utils/tar_sparse_linux.go +++ b/utils/tar_sparse_linux.go @@ -15,7 +15,7 @@ import ( var maxSparseMapJSONSize = 800 * 1024 // tarFileMaybeSparse writes file as COCOON.sparse PAX when it has holes (SEEK_HOLE/SEEK_DATA). -// Falls back to a regular tar entry on small files, unsupported FS, no holes, or oversized segment map. +// Falls back to a regular tar entry on empty files, unsupported FS, no holes, or oversized segment map. func tarFileMaybeSparse(tw *tar.Writer, path, nameInTar string) error { f, err := os.Open(path) //nolint:gosec if err != nil { diff --git a/utils/tar_test.go b/utils/tar_test.go index 10cc7884..b8b087cf 100644 --- a/utils/tar_test.go +++ b/utils/tar_test.go @@ -4,9 +4,11 @@ import ( "archive/tar" "bytes" "encoding/json" + "errors" "io" "os" "path/filepath" + "slices" "strconv" "strings" "testing" @@ -61,7 +63,7 @@ func TestTarFile(t *testing.T) { if !bytes.Equal(got, content) { t.Errorf("content mismatch: got %q, want %q", got, content) } - if _, err := tr.Next(); err != io.EOF { + if _, err := tr.Next(); !errors.Is(err, io.EOF) { t.Errorf("expected EOF, got %v", err) } } @@ -168,7 +170,7 @@ func TestTarDir_Empty(t *testing.T) { tw.Close() //nolint:errcheck tr := tar.NewReader(&buf) - if _, err := tr.Next(); err != io.EOF { + if _, err := tr.Next(); !errors.Is(err, io.EOF) { t.Errorf("expected EOF for empty dir, got %v", err) } } @@ -351,9 +353,7 @@ func TestExtractTar_RoundTrip(t *testing.T) { } } -// makeTarSparse builds a tar archive containing one file stored in our custom -// COCOON.sparse PAX format. Only the bytes described by segments are stored; -// the logical file size is realSize. +// makeTarSparse builds a tar with one COCOON.sparse PAX entry (stored bytes = segments, logical size = realSize). func makeTarSparse(t *testing.T, name string, realSize int64, segments []sparseSegment, data []byte) *bytes.Buffer { t.Helper() mapJSON, err := json.Marshal(segments) @@ -436,7 +436,7 @@ func TestExtractTar_Sparse_MultipleSegments(t *testing.T) { {Offset: 16384, Length: 4096}, {Offset: 49152, Length: 8192}, } - data := concat(seg1, seg2, seg3) + data := slices.Concat(seg1, seg2, seg3) buf := makeTarSparse(t, "multi.bin", realSize, segments, data) dir := t.TempDir() @@ -545,11 +545,9 @@ func TestExtractTar_Sparse_MixedWithRegularEntries(t *testing.T) { var buf bytes.Buffer tw := tar.NewWriter(&buf) - // Regular file first. tw.WriteHeader(&tar.Header{Name: "regular.txt", Size: 5, Typeflag: tar.TypeReg, Mode: 0o644}) //nolint:errcheck tw.Write([]byte("hello")) //nolint:errcheck - // Sparse file. tw.WriteHeader(&tar.Header{ //nolint:errcheck Name: "sparse.bin", Size: int64(len(dataContent)), @@ -568,7 +566,6 @@ func TestExtractTar_Sparse_MixedWithRegularEntries(t *testing.T) { t.Fatal(err) } - // Verify regular file. got, err := os.ReadFile(filepath.Join(dir, "regular.txt")) if err != nil { t.Fatal(err) @@ -577,7 +574,6 @@ func TestExtractTar_Sparse_MixedWithRegularEntries(t *testing.T) { t.Errorf("regular.txt: got %q", got) } - // Verify sparse file. got, err = os.ReadFile(filepath.Join(dir, "sparse.bin")) if err != nil { t.Fatal(err) @@ -638,7 +634,7 @@ func TestExtractFile_MixedZeroAndData(t *testing.T) { // Pattern: [4KB zeros] [4KB data] [4KB zeros] [4KB data] zeroBlock := make([]byte, sparseBlockSize) dataBlock := bytes.Repeat([]byte{0xCC}, sparseBlockSize) - data := concat(zeroBlock, dataBlock, zeroBlock, dataBlock) + data := slices.Concat(zeroBlock, dataBlock, zeroBlock, dataBlock) dir := t.TempDir() path := filepath.Join(dir, "mixed.bin") @@ -660,7 +656,7 @@ func TestExtractFile_EndsWithHole(t *testing.T) { // 4KB data then 4KB zeros — file must be truncated to 8KB. dataBlock := bytes.Repeat([]byte{0xDD}, sparseBlockSize) zeroBlock := make([]byte, sparseBlockSize) - data := concat(dataBlock, zeroBlock) + data := slices.Concat(dataBlock, zeroBlock) dir := t.TempDir() path := filepath.Join(dir, "endhole.bin") @@ -888,11 +884,3 @@ func TestExtractTar_RoundTrip_LargeFile(t *testing.T) { t.Error("small.txt mismatch") } } - -func concat(slices ...[]byte) []byte { - var out []byte - for _, s := range slices { - out = append(out, s...) - } - return out -}