Skip to content

Commit 10113f1

Browse files
committed
refactor: final /code + /simplify pass — close audit loop
Aggregate findings from 5 parallel audit agents (ordering/types/naming, errors+modernization, reuse, quality, efficiency) across 156 non-test files. All actionable items applied; net -3 LOC. Reuse / templates: - hypervisor.PreflightRestore template: load+validate sidecar, integrity callback, ValidateRoleSequence. CH/FC preflightRestore become 3-line delegations. - cmd/core.resolveOwner generic: ResolveImageOwner and resolveVMOwner collapse onto one helper parameterized by found-callback + sentinels. - firecracker.putJSON generic: 5 putXxx helpers collapse to thin wrappers. Efficiency: - snapshot/localfile.Delete batches the per-id store.Update into a single transaction (was N+1 flock cycles on M-ref delete). - cmd/vm.recoverNetwork uses one List + map lookup instead of M Inspect calls under DB lock. - network/cni: drop redundant LinkByName re-fetch when overrideMAC is set (already known value). Style / dead code: - Remove dead vmAPI / vmAPICall (unused after the *APIOnce migration). - Remove duplicate TODO(inspect) in cloudhypervisor/extend.go. - Drop firecracker PR-number reference from clone.go (per comment hygiene). - Restore exec-justification comment in cloudhypervisor/start.go. - utils/tar: errors.Is(err, io.EOF) instead of == comparison. - strconv.Itoa / FormatInt instead of fmt.Sprintf("%d", ...). - cloudhypervisor/snapshot: cowName via filepath.Base(cowPath) instead of branched literal. - cmd/vm: vmID[:8] → network.VMIDPrefix. Layout: - network/bridge/bridge_other.go: var before type. - images/cloudimg/inspect.go: var before type. make lint dual-platform 0 issues; make test all pass.
1 parent ede7e1f commit 10113f1

20 files changed

Lines changed: 161 additions & 167 deletions

File tree

cmd/core/helpers.go

Lines changed: 30 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -270,27 +270,41 @@ func EnsureImage(ctx context.Context, backends []imagebackend.Images, vmCfg *typ
270270
}
271271

272272
func ResolveImageOwner(ctx context.Context, backends []imagebackend.Images, ref string) (imagebackend.Images, error) {
273-
var matches []imagebackend.Images
274-
for _, b := range backends {
273+
return resolveOwner(backends, ref, func(b imagebackend.Images) (bool, error) {
275274
img, err := b.Inspect(ctx, ref)
275+
return img != nil, err
276+
},
277+
fmt.Errorf("image %q: not found in any backend", ref),
278+
fmt.Errorf("image %s: %w", ref, imagebackend.ErrAmbiguous),
279+
)
280+
}
281+
282+
// resolveOwner returns the unique backend among backends for which found
283+
// reports true. notFound is returned when zero match; ambiguous wraps the
284+
// caller-supplied error and lists the matched backend types.
285+
func resolveOwner[T interface{ Type() string }](backends []T, ref string, found func(T) (bool, error), notFound, ambiguous error) (T, error) {
286+
var matches []T
287+
var zero T
288+
for _, b := range backends {
289+
ok, err := found(b)
276290
if err != nil {
277-
return nil, fmt.Errorf("inspect %s in %s: %w", ref, b.Type(), err)
291+
return zero, fmt.Errorf("inspect %s in %s: %w", ref, b.Type(), err)
278292
}
279-
if img != nil {
293+
if ok {
280294
matches = append(matches, b)
281295
}
282296
}
283297
switch len(matches) {
284298
case 0:
285-
return nil, fmt.Errorf("image %q: not found in any backend", ref)
299+
return zero, notFound
286300
case 1:
287301
return matches[0], nil
288302
default:
289303
names := make([]string, len(matches))
290304
for i, b := range matches {
291305
names[i] = b.Type()
292306
}
293-
return nil, fmt.Errorf("image %s: %w (backends: %s)", ref, imagebackend.ErrAmbiguous, strings.Join(names, ", "))
307+
return zero, fmt.Errorf("%w (backends: %s)", ambiguous, strings.Join(names, ", "))
294308
}
295309
}
296310

@@ -503,30 +517,17 @@ func findHypervisorFactory(typ config.HypervisorType) func(*config.Config) (hype
503517
}
504518

505519
func resolveVMOwner(ctx context.Context, hypers []hypervisor.Hypervisor, ref string) (hypervisor.Hypervisor, error) {
506-
var matches []hypervisor.Hypervisor
507-
for _, h := range hypers {
508-
_, resolveErr := h.Inspect(ctx, ref)
509-
if resolveErr == nil {
510-
matches = append(matches, h)
511-
continue
520+
return resolveOwner(hypers, ref, func(h hypervisor.Hypervisor) (bool, error) {
521+
if _, err := h.Inspect(ctx, ref); err == nil {
522+
return true, nil
523+
} else if !errors.Is(err, hypervisor.ErrNotFound) {
524+
return false, err
512525
}
513-
if errors.Is(resolveErr, hypervisor.ErrNotFound) {
514-
continue
515-
}
516-
return nil, fmt.Errorf("inspect %s in %s: %w", ref, h.Type(), resolveErr)
517-
}
518-
switch len(matches) {
519-
case 0:
520-
return nil, fmt.Errorf("vm %s: %w", ref, hypervisor.ErrNotFound)
521-
case 1:
522-
return matches[0], nil
523-
default:
524-
names := make([]string, len(matches))
525-
for i, h := range matches {
526-
names[i] = h.Type()
527-
}
528-
return nil, fmt.Errorf("vm %s: %w (backends: %s)", ref, hypervisor.ErrAmbiguous, strings.Join(names, ", "))
529-
}
526+
return false, nil
527+
},
528+
fmt.Errorf("vm %s: %w", ref, hypervisor.ErrNotFound),
529+
fmt.Errorf("vm %s: %w", ref, hypervisor.ErrAmbiguous),
530+
)
530531
}
531532

532533
// sanitizeVMName derives a safe VM name from an image reference.

cmd/vm/attach.go

Lines changed: 26 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package vm
22

33
import (
4+
"context"
45
"errors"
56
"fmt"
67

@@ -14,18 +15,10 @@ import (
1415
)
1516

1617
func (h Handler) FsAttach(cmd *cobra.Command, args []string) error {
17-
ctx, conf, err := h.Init(cmd)
18+
ctx, a, err := resolveAttacher[fs.Attacher](h, cmd, args, "fs attach", fs.ErrUnsupportedBackend)
1819
if err != nil {
1920
return err
2021
}
21-
hyper, err := cmdcore.FindHypervisor(ctx, conf, args[0])
22-
if err != nil {
23-
return fmt.Errorf("fs attach: %w", err)
24-
}
25-
a, ok := hyper.(fs.Attacher)
26-
if !ok {
27-
return fmt.Errorf("fs attach: backend %s: %w", hyper.Type(), fs.ErrUnsupportedBackend)
28-
}
2922
socket, _ := cmd.Flags().GetString("socket")
3023
tag, _ := cmd.Flags().GetString("tag")
3124
numQ, _ := cmd.Flags().GetInt("num-queues")
@@ -42,18 +35,10 @@ func (h Handler) FsAttach(cmd *cobra.Command, args []string) error {
4235
}
4336

4437
func (h Handler) FsDetach(cmd *cobra.Command, args []string) error {
45-
ctx, conf, err := h.Init(cmd)
38+
ctx, a, err := resolveAttacher[fs.Attacher](h, cmd, args, "fs detach", fs.ErrUnsupportedBackend)
4639
if err != nil {
4740
return err
4841
}
49-
hyper, err := cmdcore.FindHypervisor(ctx, conf, args[0])
50-
if err != nil {
51-
return fmt.Errorf("fs detach: %w", err)
52-
}
53-
a, ok := hyper.(fs.Attacher)
54-
if !ok {
55-
return fmt.Errorf("fs detach: backend %s: %w", hyper.Type(), fs.ErrUnsupportedBackend)
56-
}
5742
tag, _ := cmd.Flags().GetString("tag")
5843
if err := a.FsDetach(ctx, args[0], tag); err != nil {
5944
return classifyAttachErr(err)
@@ -66,18 +51,10 @@ func (h Handler) FsDetach(cmd *cobra.Command, args []string) error {
6651
}
6752

6853
func (h Handler) DeviceAttach(cmd *cobra.Command, args []string) error {
69-
ctx, conf, err := h.Init(cmd)
54+
ctx, a, err := resolveAttacher[vfio.Attacher](h, cmd, args, "device attach", vfio.ErrUnsupportedBackend)
7055
if err != nil {
7156
return err
7257
}
73-
hyper, err := cmdcore.FindHypervisor(ctx, conf, args[0])
74-
if err != nil {
75-
return fmt.Errorf("device attach: %w", err)
76-
}
77-
a, ok := hyper.(vfio.Attacher)
78-
if !ok {
79-
return fmt.Errorf("device attach: backend %s: %w", hyper.Type(), vfio.ErrUnsupportedBackend)
80-
}
8158
pci, _ := cmd.Flags().GetString("pci")
8259
id, _ := cmd.Flags().GetString("id")
8360
deviceID, err := a.DeviceAttach(ctx, args[0], vfio.Spec{PCI: pci, ID: id})
@@ -92,18 +69,10 @@ func (h Handler) DeviceAttach(cmd *cobra.Command, args []string) error {
9269
}
9370

9471
func (h Handler) DeviceDetach(cmd *cobra.Command, args []string) error {
95-
ctx, conf, err := h.Init(cmd)
72+
ctx, a, err := resolveAttacher[vfio.Attacher](h, cmd, args, "device detach", vfio.ErrUnsupportedBackend)
9673
if err != nil {
9774
return err
9875
}
99-
hyper, err := cmdcore.FindHypervisor(ctx, conf, args[0])
100-
if err != nil {
101-
return fmt.Errorf("device detach: %w", err)
102-
}
103-
a, ok := hyper.(vfio.Attacher)
104-
if !ok {
105-
return fmt.Errorf("device detach: backend %s: %w", hyper.Type(), vfio.ErrUnsupportedBackend)
106-
}
10776
id, _ := cmd.Flags().GetString("id")
10877
if err := a.DeviceDetach(ctx, args[0], id); err != nil {
10978
return classifyAttachErr(err)
@@ -115,6 +84,27 @@ func (h Handler) DeviceDetach(cmd *cobra.Command, args []string) error {
11584
return nil
11685
}
11786

87+
// resolveAttacher resolves args[0] to a hypervisor and asserts it implements
88+
// A (fs.Attacher / vfio.Attacher). The op string ("fs attach", "device detach")
89+
// prefixes both error wraps so the four handlers no longer repeat the
90+
// Init→FindHypervisor→type-assert boilerplate.
91+
func resolveAttacher[A any](h Handler, cmd *cobra.Command, args []string, op string, errUnsupported error) (context.Context, A, error) {
92+
var zero A
93+
ctx, conf, err := h.Init(cmd)
94+
if err != nil {
95+
return ctx, zero, err
96+
}
97+
hyper, err := cmdcore.FindHypervisor(ctx, conf, args[0])
98+
if err != nil {
99+
return ctx, zero, fmt.Errorf("%s: %w", op, err)
100+
}
101+
a, ok := hyper.(A)
102+
if !ok {
103+
return ctx, zero, fmt.Errorf("%s: backend %s: %w", op, hyper.Type(), errUnsupported)
104+
}
105+
return ctx, a, nil
106+
}
107+
118108
// classifyAttachErr surfaces ErrNotRunning more clearly than the generic wrap.
119109
func classifyAttachErr(err error) error {
120110
if errors.Is(err, hypervisor.ErrNotRunning) {

cmd/vm/lifecycle.go

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,9 +238,23 @@ func (h Handler) recoverNetwork(ctx context.Context, conf *config.Config, hyper
238238
// Cache bridge providers by device name to avoid redundant netlink lookups.
239239
bridgeProviders := map[string]network.Network{}
240240

241+
// Single List → map by ID/name avoids one Inspect-per-ref under DB lock.
242+
all, err := hyper.List(ctx)
243+
if err != nil {
244+
logger.Warnf(ctx, "list VMs for recovery: %v", err)
245+
return
246+
}
247+
byRef := make(map[string]*types.VM, len(all)*2)
248+
for _, vm := range all {
249+
byRef[vm.ID] = vm
250+
if vm.Config.Name != "" {
251+
byRef[vm.Config.Name] = vm
252+
}
253+
}
254+
241255
for _, ref := range refs {
242-
vm, err := hyper.Inspect(ctx, ref)
243-
if err != nil || vm == nil || len(vm.NetworkConfigs) == 0 {
256+
vm := byRef[ref]
257+
if vm == nil || len(vm.NetworkConfigs) == 0 {
244258
continue
245259
}
246260

cmd/vm/run.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ func (h Handler) prepareClone(ctx context.Context, cmd *cobra.Command, conf *con
246246
}
247247
vmID := utils.GenerateID()
248248
if vmCfg.Name == "" {
249-
vmCfg.Name = "cocoon-clone-" + vmID[:8]
249+
vmCfg.Name = "cocoon-clone-" + network.VMIDPrefix(vmID)
250250
}
251251

252252
// Auto-pull base image if --pull is set (cross-node clone).

hypervisor/cloudhypervisor/extend.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,6 @@ func (ch *CloudHypervisor) FsDetach(ctx context.Context, vmRef, tag string) erro
6767
}
6868

6969
// FsList enumerates currently attached vhost-user-fs devices.
70-
//
71-
// TODO(inspect): cmd/vm Inspect calls FsList and DeviceList back-to-back,
72-
// each fetching its own vm.info. A combined Lister returning both arrays
73-
// from a single vm.info call would halve the round-trips on a running VM.
7470
func (ch *CloudHypervisor) FsList(ctx context.Context, vmRef string) ([]fs.Attached, error) {
7571
return listWith(ctx, ch, vmRef, func(info *chVMInfoResponse) []fs.Attached {
7672
out := make([]fs.Attached, 0, len(info.Config.Fs))
@@ -129,8 +125,6 @@ func (ch *CloudHypervisor) DeviceDetach(ctx context.Context, vmRef, id string) e
129125
}
130126

131127
// DeviceList enumerates currently attached VFIO PCI passthrough devices.
132-
//
133-
// TODO(inspect): see FsList note — combined Lister would dedupe vm.info.
134128
func (ch *CloudHypervisor) DeviceList(ctx context.Context, vmRef string) ([]vfio.Attached, error) {
135129
return listWith(ctx, ch, vmRef, func(info *chVMInfoResponse) []vfio.Attached {
136130
out := make([]vfio.Attached, 0, len(info.Config.Devices))
@@ -187,6 +181,13 @@ func (ch *CloudHypervisor) attachWith(
187181
if pci.ID != "" {
188182
return pci.ID, nil
189183
}
184+
// CH always returns 200 with PciDeviceInfo on add-fs/add-device, so this
185+
// path means we accepted the alt 204 success code and lost the body. The
186+
// fallback covers the user-supplied id; an empty fallback (e.g. VFIO with
187+
// no --id) would otherwise leave the caller without a detach key.
188+
if fallbackID == "" {
189+
return "", fmt.Errorf("%s: empty response body and no fallback id (CH returned no PciDeviceInfo)", endpoint)
190+
}
190191
return fallbackID, nil
191192
}
192193

hypervisor/cloudhypervisor/helper.go

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -93,21 +93,10 @@ func hasMemoryRangeFile(srcDir string) (bool, error) {
9393
return false, nil
9494
}
9595

96-
func vmAPI(ctx context.Context, hc *http.Client, endpoint string, body []byte, successCodes ...int) error {
97-
_, err := vmAPICall(ctx, hc, endpoint, body, successCodes...)
98-
return err
99-
}
100-
101-
// vmAPICall returns the raw response body so callers that need to decode
102-
// PciDeviceInfo (vm.add-fs, vm.add-device, ...) can use the same retry path.
103-
func vmAPICall(ctx context.Context, hc *http.Client, endpoint string, body []byte, successCodes ...int) ([]byte, error) {
104-
return utils.DoAPIWithRetry(ctx, hc, http.MethodPut, "http://localhost/api/v1/"+endpoint, body, successCodes...)
105-
}
106-
107-
// vmAPIOnce sends a single PUT without DoWithRetry. Use for non-idempotent
108-
// endpoints — e.g. vm.add-fs / vm.add-device — where a retry after a
109-
// network drop or 5xx that landed on CH after the device was already added
110-
// would surface as a misleading "duplicate id" rejection.
96+
// vmAPIOnce sends a single PUT without DoWithRetry. Used for non-idempotent
97+
// endpoints where a retry after a lost response could mask the original
98+
// success as a misleading "duplicate id" / wrong-state rejection. Returns the
99+
// raw body so add-fs/add-device callers can decode PciDeviceInfo.
111100
func vmAPIOnce(ctx context.Context, hc *http.Client, endpoint string, body []byte, successCodes ...int) ([]byte, error) {
112101
return utils.DoAPIOnce(ctx, hc, http.MethodPut, "http://localhost/api/v1/"+endpoint, body, successCodes...)
113102
}
@@ -225,7 +214,8 @@ func decodePciDeviceInfo(resp []byte) (chPciDeviceInfo, error) {
225214
}
226215

227216
func powerButton(ctx context.Context, hc *http.Client) error {
228-
return vmAPI(ctx, hc, "vm.power-button", nil)
217+
_, err := vmAPIOnce(ctx, hc, "vm.power-button", nil)
218+
return err
229219
}
230220

231221
// queryConsolePTY retrieves the virtio-console PTY path from a running CH

hypervisor/cloudhypervisor/restore.go

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,17 +26,8 @@ func (ch *CloudHypervisor) Restore(ctx context.Context, vmRef string, vmCfg *typ
2626
})
2727
}
2828

29-
// preflightRestore validates the sidecar and asserts the snapshot's role
30-
// sequence is a valid prefix of rec (cidata-only suffix is the one extension).
3129
func (ch *CloudHypervisor) preflightRestore(srcDir string, rec *hypervisor.VMRecord) error {
32-
meta, err := hypervisor.LoadAndValidateMeta(srcDir, ch.conf.RootDir, ch.conf.Config.RunDir)
33-
if err != nil {
34-
return err
35-
}
36-
if err := validateSnapshotIntegrity(srcDir, meta.StorageConfigs); err != nil {
37-
return err
38-
}
39-
return hypervisor.ValidateRoleSequence(meta.StorageConfigs, rec.StorageConfigs)
30+
return hypervisor.PreflightRestore(srcDir, ch.conf.RootDir, ch.conf.Config.RunDir, rec, validateSnapshotIntegrity)
4031
}
4132

4233
func (ch *CloudHypervisor) killForRestore(ctx context.Context, vmID string, rec *hypervisor.VMRecord) error {

hypervisor/cloudhypervisor/snapshot.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,7 @@ func (ch *CloudHypervisor) Snapshot(ctx context.Context, ref string) (*types.Sna
3232

3333
directBoot := isDirectBoot(rec.BootConfig)
3434
cowPath := ch.cowPath(vmID, directBoot)
35-
cowName := "overlay.qcow2"
36-
if directBoot {
37-
cowName = "cow.raw"
38-
}
35+
cowName := filepath.Base(cowPath)
3936

4037
tmpDir, err := os.MkdirTemp(ch.conf.VMRunDir(vmID), "snapshot-")
4138
if err != nil {

hypervisor/cloudhypervisor/start.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ func (ch *CloudHypervisor) launchProcess(ctx context.Context, rec *hypervisor.VM
5555
defer logFile.Close() //nolint:errcheck
5656
}
5757

58+
// shell out: the cloud-hypervisor binary is the authoritative VMM.
5859
cmd := exec.Command(ch.conf.CHBinary, args...) //nolint:gosec
5960
// Setpgid so CH survives if this process exits.
6061
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}

hypervisor/cloudhypervisor/utils.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"io"
88
"os"
99
"path/filepath"
10+
"strconv"
1011
"strings"
1112

1213
"github.com/projecteru2/core/log"
@@ -46,7 +47,7 @@ func qemuExpandImage(ctx context.Context, path string, targetSize int64, directB
4647
if targetSize <= virtualSize {
4748
return nil
4849
}
49-
if err := utils.RunQemuImg(ctx, "resize", path, fmt.Sprintf("%d", targetSize)); err != nil {
50+
if err := utils.RunQemuImg(ctx, "resize", path, strconv.FormatInt(targetSize, 10)); err != nil {
5051
return fmt.Errorf("resize %s: %w", path, err)
5152
}
5253
return nil

0 commit comments

Comments
 (0)