Skip to content

Commit 42cae70

Browse files
committed
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).
1 parent a17c7d0 commit 42cae70

75 files changed

Lines changed: 212 additions & 338 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

cmd/core/ensure_image_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,7 @@ func (f *fakeImageBackend) Config(context.Context, []*types.VMConfig) ([][]*type
4242
return nil, nil, nil
4343
}
4444

45-
// Regression guard for issue 37: pinned digest with no local hit must force-pull
46-
// to bypass cloudimg's URL-keyed cache.
45+
// Issue 37 regression guard: a pinned digest with no local hit must force-pull past cloudimg's URL-keyed cache.
4746
func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) {
4847
const (
4948
url = "https://epoch.example/dl/simular/win11"
@@ -98,8 +97,7 @@ func TestEnsureImage_ForceWhenDigestPinned(t *testing.T) {
9897
}
9998
}
10099

101-
// A cloudimg ref without an http(s) scheme reaching Pull surfaces as
102-
// `unsupported protocol scheme` from http.Get; the shape guard short-circuits.
100+
// A non-http(s) cloudimg ref reaching Pull would surface http.Get's `unsupported protocol scheme`; the shape guard short-circuits first.
103101
func TestEnsureImage_SkipsBadShape(t *testing.T) {
104102
tests := []struct {
105103
name string

cmd/core/helpers.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,7 @@ func EnsureImage(ctx context.Context, backends []imagebackend.Images, vmCfg *typ
9898
if img != nil {
9999
return // exact image version exists locally
100100
}
101-
// Pull by digest reference when available — ensures we get the exact
102-
// version recorded at snapshot time, not whatever the tag points to now.
101+
// Pull by digest when available — the tag may point at a different manifest than at snapshot time.
103102
pullRef := digestPullRef(vmCfg.Image, vmCfg.ImageDigest, vmCfg.ImageType)
104103
if shapeErr := validateRefShape(pullRef, vmCfg.ImageType); shapeErr != nil {
105104
logger.Warnf(ctx, "skipping auto-pull of %s: %v — pre-pull manually if missing", pullRef, shapeErr)

cmd/core/metering.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func buildRecorder(ctx context.Context, conf *config.Config) metering.Recorder {
4141
case "stderr":
4242
return meteringstderr.New()
4343
default:
44-
log.WithFunc("core.MeteringRecorder").Warnf(ctx, "unknown metering backend %q; using nop", conf.Metering.Backend)
44+
log.WithFunc("core.buildRecorder").Warnf(ctx, "unknown metering backend %q; using nop", conf.Metering.Backend)
4545
return metering.NopRecorder{}
4646
}
4747
}

cmd/images/handler_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ import (
88
"os"
99
"path/filepath"
1010
"testing"
11-
)
1211

13-
var qcow2Magic = []byte{'Q', 'F', 'I', 0xfb}
12+
"github.com/cocoonstack/cocoon/utils"
13+
)
1414

1515
func gzipWrap(t *testing.T, data []byte) []byte {
1616
t.Helper()
@@ -55,7 +55,7 @@ func writeTempFile(t *testing.T, dir, name string, data []byte) string {
5555
}
5656

5757
func TestDetectReader(t *testing.T) {
58-
qcow2Data := append(qcow2Magic, make([]byte, 100)...)
58+
qcow2Data := append(utils.Qcow2Magic, make([]byte, 100)...)
5959
tarData := []byte("this is a tar-like stream of data, not really tar but not qcow2 either")
6060

6161
tests := []struct {
@@ -155,7 +155,7 @@ func TestDetectReader_GzipPreservesFullContent(t *testing.T) {
155155

156156
func TestDetectLocalImportSource(t *testing.T) {
157157
dir := t.TempDir()
158-
qcow2Path := writeTempFile(t, dir, "image.qcow2", append(qcow2Magic, make([]byte, 32)...))
158+
qcow2Path := writeTempFile(t, dir, "image.qcow2", append(utils.Qcow2Magic, make([]byte, 32)...))
159159
tarPath := writeTempFile(t, dir, "image.tar", tarWrap(t, "payload.txt", []byte("payload")))
160160
gzipTarPath := writeTempFile(t, dir, "image.tar.gz", gzipWrap(t, tarWrap(t, "payload.txt", []byte("payload"))))
161161
invalidPath := writeTempFile(t, dir, "invalid.bin", []byte("not an archive"))
@@ -193,7 +193,7 @@ func TestDetectLocalImportSource(t *testing.T) {
193193

194194
func TestPlanLocalImportPreservesAllFiles(t *testing.T) {
195195
dir := t.TempDir()
196-
part1 := writeTempFile(t, dir, "part-1.qcow2", append(qcow2Magic, make([]byte, 8)...))
196+
part1 := writeTempFile(t, dir, "part-1.qcow2", append(utils.Qcow2Magic, make([]byte, 8)...))
197197
part2 := writeTempFile(t, dir, "part-2.qcow2", []byte("second-part"))
198198

199199
plan, err := planLocalImport([]string{part1, part2})

cmd/images/import.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/cocoonstack/cocoon/progress"
2121
cloudimgProgress "github.com/cocoonstack/cocoon/progress/cloudimg"
2222
ociProgress "github.com/cocoonstack/cocoon/progress/oci"
23+
"github.com/cocoonstack/cocoon/utils"
2324
)
2425

2526
func (h Handler) Import(cmd *cobra.Command, args []string) error {
@@ -226,10 +227,10 @@ func detectLocalImportSource(filePath string) (importSourceKind, error) {
226227
return 0, fmt.Errorf("peek %s: %w", filePath, readErr)
227228
}
228229

229-
if n >= 2 && bytes.Equal(magic[:2], []byte{0x1f, 0x8b}) {
230+
if n >= 2 && bytes.Equal(magic[:2], utils.GzipMagic) {
230231
return importSourceStream, nil
231232
}
232-
if n >= 4 && bytes.Equal(magic[:4], []byte{'Q', 'F', 'I', 0xfb}) {
233+
if n >= 4 && bytes.Equal(magic[:4], utils.Qcow2Magic) {
233234
return importSourceQcow2, nil
234235
}
235236

@@ -270,7 +271,7 @@ func detectReader(r io.Reader) (io.Reader, imageType, func(), error) {
270271
return nil, 0, nil, fmt.Errorf("peek content: %w", err)
271272
}
272273

273-
if cpeek[0] == 'Q' && cpeek[1] == 'F' && cpeek[2] == 'I' && cpeek[3] == 0xfb {
274+
if bytes.HasPrefix(cpeek, utils.Qcow2Magic) {
274275
return inner, imageTypeQcow2, cleanup, nil
275276
}
276277

cmd/snapshot/commands.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func Command(h Actions) *cobra.Command {
6161
}
6262
exportCmd.Flags().StringP("output", "o", "", "output file path (default: <name-or-id>.tar)")
6363
exportCmd.Flags().Bool("gzip", false, "compress output with gzip")
64-
exportCmd.Flags().String("to-dir", "", "export into a directory (must be empty/absent) instead of a tar; pairs with `vm clone --from-dir`")
64+
exportCmd.Flags().String("to-dir", "", "export into a directory (must be empty/absent) instead of a tar; pairs with 'vm clone --from-dir'")
6565
exportCmd.MarkFlagsMutuallyExclusive("to-dir", "output")
6666
exportCmd.MarkFlagsMutuallyExclusive("to-dir", "gzip")
6767

cmd/snapshot/handler.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,13 +110,10 @@ func (h Handler) List(cmd *cobra.Command, _ []string) error {
110110
}
111111

112112
if filterIDs != nil {
113-
filtered := snapshots[:0]
114-
for _, s := range snapshots {
115-
if _, ok := filterIDs[s.ID]; ok {
116-
filtered = append(filtered, s)
117-
}
118-
}
119-
snapshots = filtered
113+
snapshots = slices.DeleteFunc(snapshots, func(s *types.Snapshot) bool {
114+
_, ok := filterIDs[s.ID]
115+
return !ok
116+
})
120117
}
121118

122119
if len(snapshots) == 0 {

cmd/vm/debug.go

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -109,48 +109,46 @@ func printFCDebug(configs []*types.StorageConfig, boot *types.BootConfig, vmCfg
109109
fmt.Println("# Configure via REST API (use curl or similar):")
110110
sock := fmt.Sprintf("/tmp/fc-%s.sock", vmCfg.Name)
111111

112-
fmt.Printf("# 1. Machine config\n")
112+
fmt.Println("# 1. Machine config")
113113
fmt.Printf("curl --unix-socket %s -X PUT http://localhost/machine-config \\\n", sock)
114114
fmt.Printf(" -d '{\"vcpu_count\": %d, \"mem_size_mib\": %d}'\n", vmCfg.CPU, memMiB)
115115
fmt.Println()
116116

117-
fmt.Printf("# 2. Boot source\n")
117+
fmt.Println("# 2. Boot source")
118118
fmt.Printf("curl --unix-socket %s -X PUT http://localhost/boot-source \\\n", sock)
119119
fmt.Printf(" -d '{\"kernel_image_path\": \"%s\", \"initrd_path\": \"%s\", \"boot_args\": \"%s\"}'\n",
120120
boot.KernelPath, boot.InitrdPath, cmdline)
121121
fmt.Println()
122122

123-
fmt.Printf("# 3. Drives\n")
123+
fmt.Println("# 3. Drives")
124124
for i, sc := range configs {
125125
fmt.Printf("curl --unix-socket %s -X PUT http://localhost/drives/drive_%d \\\n", sock, i)
126126
fmt.Printf(" -d '{\"drive_id\": \"drive_%d\", \"path_on_host\": \"%s\", \"is_root_device\": false, \"is_read_only\": %t}'\n",
127127
i, sc.Path, sc.RO)
128128
}
129-
// COW drive
130129
fmt.Printf("curl --unix-socket %s -X PUT http://localhost/drives/drive_%d \\\n", sock, len(configs))
131130
fmt.Printf(" -d '{\"drive_id\": \"drive_%d\", \"path_on_host\": \"%s\", \"is_root_device\": false, \"is_read_only\": false}'\n",
132131
len(configs), cowPath)
133132
fmt.Println()
134133

135134
if size, ok := hypervisor.BalloonSize(vmCfg.Memory, vmCfg.Windows); ok {
136-
fmt.Printf("# 4. Balloon\n")
135+
fmt.Println("# 4. Balloon")
137136
fmt.Printf("curl --unix-socket %s -X PUT http://localhost/balloon \\\n", sock)
138137
fmt.Printf(" -d '{\"amount_mib\": %d, \"deflate_on_oom\": true, \"free_page_reporting\": true}'\n", size>>20) //nolint:mnd
139138
fmt.Println()
140139
}
141140

142-
fmt.Printf("# 5. Start\n")
141+
fmt.Println("# 5. Start")
143142
fmt.Printf("curl --unix-socket %s -X PUT http://localhost/actions \\\n", sock)
144-
fmt.Printf(" -d '{\"action_type\": \"InstanceStart\"}'\n")
143+
fmt.Println(" -d '{\"action_type\": \"InstanceStart\"}'")
145144
}
146145

147146
func buildCHDebugSpec(cmd *cobra.Command, storageConfigs []*types.StorageConfig, boot *types.BootConfig, vmCfg *types.VMConfig) chDebugSpec {
148147
maxCPU, _ := cmd.Flags().GetInt("max-cpu")
149148
balloon, _ := cmd.Flags().GetInt("balloon")
150149
cowPath, _ := cmd.Flags().GetString("cow")
151150
chBin, _ := cmd.Flags().GetString("ch")
152-
// Mirror runtime gating: Windows / sub-MinBalloon VMs never get balloon,
153-
// even if the user passed --balloon, so debug output stays truthful.
151+
// Mirror runtime gating: Windows / sub-MinBalloon VMs never get balloon even with --balloon, so debug output stays truthful.
154152
size, ok := hypervisor.BalloonSize(vmCfg.Memory, vmCfg.Windows)
155153
switch {
156154
case !ok:
@@ -196,11 +194,11 @@ func printCHDebug(s chDebugSpec) {
196194
fmt.Printf("%s \\\n", s.CHBin)
197195
fmt.Printf(" --kernel %s \\\n", s.Boot.KernelPath)
198196
fmt.Printf(" --initramfs %s \\\n", s.Boot.InitrdPath)
199-
fmt.Printf(" --disk")
197+
fmt.Print(" --disk")
200198
for _, d := range diskArgs {
201199
fmt.Printf(" \\\n \"%s\"", d)
202200
}
203-
fmt.Printf(" \\\n")
201+
fmt.Print(" \\\n")
204202
fmt.Printf(" --cmdline \"%s\" \\\n", cmdline)
205203
} else {
206204
if s.CowPath == "" {
@@ -216,7 +214,7 @@ func printCHDebug(s chDebugSpec) {
216214
fmt.Printf("# Launch VM: %s (image: %s, boot: UEFI firmware)\n", s.VMCfg.Name, s.VMCfg.Image)
217215
fmt.Printf("%s \\\n", s.CHBin)
218216
fmt.Printf(" --firmware %s \\\n", s.Boot.FirmwarePath)
219-
fmt.Printf(" --disk \\\n")
217+
fmt.Print(" --disk \\\n")
220218
diskArgs := cloudhypervisor.DebugDiskCLIArgs([]*types.StorageConfig{{Path: s.CowPath, RO: false}}, cpu, diskQueueSize, noDirectIO)
221219
fmt.Printf(" \"%s\" \\\n", diskArgs[0])
222220
}
@@ -234,10 +232,10 @@ func printCommonCHArgs(s chDebugSpec) {
234232
}
235233
fmt.Printf(" --cpus boot=%d,max=%d%s \\\n", s.VMCfg.CPU, s.MaxCPU, cpuExtra)
236234
fmt.Printf(" --memory size=%dM%s \\\n", s.VMCfg.Memory>>20, memExtra) //nolint:mnd
237-
fmt.Printf(" --rng src=/dev/urandom \\\n")
235+
fmt.Print(" --rng src=/dev/urandom \\\n")
238236
if s.Balloon > 0 {
239237
fmt.Printf(" --balloon size=%dM,deflate_on_oom=on,free_page_reporting=on \\\n", s.Balloon)
240238
}
241-
fmt.Printf(" --watchdog \\\n")
242-
fmt.Printf(" --serial tty --console off\n")
239+
fmt.Print(" --watchdog \\\n")
240+
fmt.Println(" --serial tty --console off")
243241
}

cmd/vm/exec_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,7 @@ func TestReadHybridVsockReply(t *testing.T) {
7777
}
7878
}
7979

80-
// TestDialHybridVsock_ConnectHandshake spins up an in-process listener that
81-
// speaks the CH/FC hybrid vsock dialect (CONNECT <port>\n → OK <port>\n).
80+
// TestDialHybridVsock_ConnectHandshake drives the CH/FC hybrid vsock dialect (CONNECT <port>\n → OK <port>\n) against an in-process listener.
8281
func TestDialHybridVsock_ConnectHandshake(t *testing.T) {
8382
// macOS caps unix socket paths at ~104 bytes, so t.TempDir() (long
8483
// /var/folders/... path) can overflow. Use os.CreateTemp + immediate unlink.

cmd/vm/lifecycle.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ func finishRoutedCmd(ctx context.Context, cmd *cobra.Command, logTag, name, past
343343
}
344344

345345
func batchRoutedCmd(ctx context.Context, cmd *cobra.Command, name, pastTense string, routed map[hypervisor.Hypervisor][]string, fn func(hypervisor.Hypervisor, []string) ([]string, error)) error {
346-
logTag := "cmd." + name
346+
logTag := "cmd.vm." + name
347347
allDone, lastErr := runRoutedLoop(ctx, logTag, pastTense, cmdcore.WantJSON(cmd), routed, fn)
348348
return finishRoutedCmd(ctx, cmd, logTag, name, pastTense, allDone, lastErr)
349349
}

0 commit comments

Comments
 (0)