review: whole-repo round — net rollback, stray-helper reap, NBD busy check, VNC password rules; pin cocoon

cmd/image/handler.go

@@ -23,7 +23,8 @@ type Handler struct{}
 func NewHandler() *Handler { return &Handler{} }
 
 func (h *Handler) Pull(cmd *cobra.Command, args []string) error {
-	ctx, s, err := home.OpenStore(cmd)
+	ctx := cliutil.CommandContext(cmd)
+	s, err := home.OpenStore(ctx, cmd)
 	if err != nil {
 		return err
 	}
@@ -68,7 +69,8 @@ func (h *Handler) Pull(cmd *cobra.Command, args []string) error {
 }
 
 func (h *Handler) List(cmd *cobra.Command, _ []string) error {
-	ctx, s, err := home.OpenStore(cmd)
+	ctx := cliutil.CommandContext(cmd)
+	s, err := home.OpenStore(ctx, cmd)
 	if err != nil {
 		return err
 	}
@@ -77,17 +79,18 @@ func (h *Handler) List(cmd *cobra.Command, _ []string) error {
 		return err
 	}
 	return cliutil.OutputFormatted(cmd, imgs, func(w *tabwriter.Writer) {
-		fmt.Fprintln(w, "NAME\tTYPE\tSIZE\tDIGEST\tCREATED") //nolint:errcheck
+		fmt.Fprintln(w, "NAME\tTYPE\tSIZE\tDIGEST\tCREATED") //nolint:errcheck // the tabwriter flush reports the write error
 		for _, img := range imgs {
-			fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", //nolint:errcheck
+			fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\n", //nolint:errcheck // the tabwriter flush reports the write error
 				img.Name, img.Type, cliutil.FormatSize(img.Size),
 				shortDigest(img.ID), img.CreatedAt.Local().Format(time.DateTime))
 		}
 	})
 }
 
 func (h *Handler) Inspect(cmd *cobra.Command, args []string) error {
-	ctx, s, err := home.OpenStore(cmd)
+	ctx := cliutil.CommandContext(cmd)
+	s, err := home.OpenStore(ctx, cmd)
 	if err != nil {
 		return err
 	}
@@ -102,7 +105,8 @@ func (h *Handler) Inspect(cmd *cobra.Command, args []string) error {
 }
 
 func (h *Handler) RM(cmd *cobra.Command, args []string) error {
-	ctx, s, err := home.OpenStore(cmd)
+	ctx := cliutil.CommandContext(cmd)
+	s, err := home.OpenStore(ctx, cmd)
 	if err != nil {
 		return err
 	}

cmd/vm/clone.go

@@ -41,7 +41,10 @@ func (h *Handler) Clone(cmd *cobra.Command, args []string) error {
 }
 
 func (h *Handler) clone(cmd *cobra.Command, srcRec *record, name string) (retErr error) {
-	netMode, _ := cmd.Flags().GetString("net")
+	netMode := srcRec.NetMode
+	if cmd.Flags().Changed("net") {
+		netMode, _ = cmd.Flags().GetString("net")
+	}
 	vnc, _ := cmd.Flags().GetInt("vnc")
 	vncPass, _ := cmd.Flags().GetString("vnc-password")
 	if err := requireCNIVNCPassword(netMode == netCNI, vnc, vncPass); err != nil {
@@ -86,7 +89,7 @@ func (h *Handler) clone(cmd *cobra.Command, srcRec *record, name string) (retErr
 	if err != nil {
 		return err
 	}
-	copied, err := copyDataDisks(dir, srcRec.DataDisks)
+	copied, err := copyDataDisks(ctx, dir, srcRec.DataDisks)
 	if err != nil {
 		return err
 	}
@@ -128,6 +131,11 @@ func (h *Handler) clone(cmd *cobra.Command, srcRec *record, name string) (retErr
 		r.MAC = srcRec.MAC // prepareOpenCore only sets a fresh MAC for fresh identities
 	}
 	r.NetMode = netMode
+	if bridge, _ := cmd.Flags().GetString("bridge"); bridge != "" {
+		r.BridgeDev = bridge
+	} else if !cmd.Flags().Changed("net") {
+		r.BridgeDev = srcRec.BridgeDev
+	}
 	tapFlag, _ := cmd.Flags().GetString("tap")
 	r.Tap = tapFlag
 	if err = applyNet(cmd, r); err != nil {

cmd/vm/commands.go

@@ -33,7 +33,7 @@ func Command(h *Handler) *cobra.Command {
 		RunE:  h.Start,
 	}
 	startCmd.Flags().Int("vnc", -1, "VNC display number for this start only (n => port 590n); omit to keep VNC off")
-	startCmd.Flags().String("vnc-password", "", "VNC password for this start (≤8 chars, QEMU password auth)")
+	startCmd.Flags().String("vnc-password", "", "VNC password for this start (≤8 bytes, QEMU password auth)")
 
 	stopCmd := &cobra.Command{
 		Use:   "stop VM [VM...]",
@@ -89,11 +89,11 @@ func Command(h *Handler) *cobra.Command {
 	}
 	restoreCmd.Flags().String("tag", "", "snapshot tag to restore (default: newest)")
 	restoreCmd.Flags().Bool("force", false, "stop the VM, restore, then relaunch if it was running")
-	restoreCmd.Flags().String("vnc-password", "", "VNC password for the relaunch of a running VM whose display is password-gated (≤8 chars)")
+	restoreCmd.Flags().String("vnc-password", "", "VNC password for the relaunch of a running VM whose display is password-gated (≤8 bytes)")
 
 	cloneCmd := &cobra.Command{
 		Use:   "clone SRC",
-		Short: "Clone a stopped VM: fresh CoW overlay on the shared base + a unique Apple identity + its own TAP",
+		Short: "Clone a stopped VM: fresh CoW overlay on the shared base + a unique Apple identity + its own network endpoint (--net inherited from the source)",
 		Args:  cobra.ExactArgs(1),
 		RunE:  h.Clone,
 	}
@@ -119,7 +119,7 @@ func addVMFlags(cmd *cobra.Command) {
 	cmd.Flags().String("ovmf-code", "", "OVMF_CODE firmware (default: <state-dir>/firmware/OVMF_CODE.fd)")
 	cmd.Flags().String("ovmf-vars", "", "OVMF_VARS template, copied per-VM (default: <state-dir>/firmware/OVMF_VARS.fd)")
 	cmd.Flags().Bool("random-smbios", false, "inject a unique Apple SMBIOS identity per VM (serial/MLB/UUID/ROM)")
-	cmd.Flags().String("vnc-password", "", "set a VNC password (≤8 chars) so macOS Screen Sharing can connect (QEMU password auth)")
+	cmd.Flags().String("vnc-password", "", "set a VNC password (≤8 bytes) so macOS Screen Sharing can connect (QEMU password auth)")
 	cmd.Flags().String("net", "user", "network mode: user (SLIRP + --ssh-port hostfwd) | tap | cni | bridge (cocoon auto-creates the TAP, Linux only)")
 	cmd.Flags().String("tap", "", "pre-created host TAP ifname (skips auto-create; e.g. an existing bridge port / cocoon CNI tap)")
 	cmd.Flags().String("bridge", "", "existing Linux bridge to enslave the auto-created TAP to (--net tap|bridge)")

cmd/vm/datadisk.go

@@ -113,11 +113,11 @@ func createDataDisks(ctx context.Context, dir string, specs []types.DataDiskSpec
 	return paths, nil
 }
 
-func copyDataDisks(dir string, src []string) ([]string, error) {
+func copyDataDisks(ctx context.Context, dir string, src []string) ([]string, error) {
 	paths := make([]string, 0, len(src))
 	for _, srcPath := range src {
 		dst := filepath.Join(dir, filepath.Base(srcPath))
-		if err := utils.ReflinkCopy(dst, srcPath, utils.Sync); err != nil {
+		if err := utils.ReflinkCopy(ctx, dst, srcPath, utils.Sync); err != nil {
 			return nil, fmt.Errorf("copy data disk %s: %w", filepath.Base(srcPath), err)
 		}
 		paths = append(paths, dst)

cmd/vm/datadisk_test.go

@@ -36,7 +36,6 @@ func TestParseDataDisks(t *testing.T) {
 		{name: "directio rejected on macOS", raw: []string{"size=1G,directio=on"}, wantErr: true},
 		{name: "over the four-disk cap", raw: []string{"size=1G", "size=1G", "size=1G", "size=1G", "size=1G"}, wantErr: true},
 
-		// clone reserves the copied disks' names: collisions error and reserved count against the cap
 		{name: "collides with a reserved name", raw: []string{"name=data1,size=1G"}, reserved: []string{"data1"}, wantErr: true},
 		{name: "auto name skips reserved", raw: []string{"size=1G"}, reserved: []string{"data1"}, wantNames: []string{"data2"}, wantSizes: []int64{gib}},
 		{name: "reserved fills the cap", raw: []string{"size=1G", "size=1G"}, reserved: []string{"data1", "data2", "data3"}, wantErr: true},
@@ -69,7 +68,6 @@ func TestParseDataDisks(t *testing.T) {
 }
 
 func TestDataDiskNameRoundTrip(t *testing.T) {
-	// clone recovers reserved names from the copied disks' paths, so this must invert dataDiskPath
 	for _, name := range []string{"data1", "logs", "a-b_c"} {
 		if got := dataDiskName(dataDiskPath("/vm/dir", name)); got != name {
 			t.Errorf("dataDiskName(dataDiskPath(%q)) = %q", name, got)

cmd/vm/handler_test.go

@@ -41,7 +41,6 @@ func TestStartAlreadyRunningIsIdempotent(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// A real process whose argv0+args satisfy isRunning without qemu/KVM.
 	fakeQEMU := filepath.Join(t.TempDir(), qemuBinary)
 	if err := os.Symlink("/bin/sh", fakeQEMU); err != nil {
 		t.Fatal(err)
@@ -155,9 +154,7 @@ func TestImagesToSnapshot(t *testing.T) {
 		rec  *record
 		want []string
 	}{
-		// raw .fd NVRAM can't hold internal snapshots, so only the disk is captured
 		{"raw nvram captures disk only", &record{Disk: "/v/disk.qcow2", OVMFVars: "/v/OVMF_VARS.fd"}, []string{"/v/disk.qcow2"}},
-		// a qcow2 NVRAM rolls back too
 		{"qcow2 nvram captures both", &record{Disk: "/v/disk.qcow2", OVMFVars: "/v/OVMF_VARS.qcow2"}, []string{"/v/disk.qcow2", "/v/OVMF_VARS.qcow2"}},
 	}
 	for _, tt := range tests {
@@ -169,7 +166,6 @@ func TestImagesToSnapshot(t *testing.T) {
 	}
 }
 
-// auto-create/CNI/bridge need Linux + CAP_NET_ADMIN; they are smoke-tested on the testbed.
 func TestPrepareNetNoProvision(t *testing.T) {
 	tests := []struct {
 		name                        string

cmd/vm/lifecycle.go

@@ -19,38 +19,11 @@ import (
 )
 
 func (h *Handler) Create(cmd *cobra.Command, args []string) error {
-	name := requestedVMName(cmd, "macos-"+time.Now().Format("20060102-150405"))
-	dir, err := home.VMDir(cmd, name)
-	if err != nil {
-		return err
-	}
-	return withVMLock(cliutil.CommandContext(cmd), dir, func() error {
-		r, err := h.create(cmd, args[0], name)
-		if err != nil {
-			return err
-		}
-		fmt.Println(r.Name)
-		return nil
-	})
+	return h.createVM(cmd, args[0], false)
 }
 
 func (h *Handler) Run(cmd *cobra.Command, args []string) error {
-	name := requestedVMName(cmd, "macos-"+time.Now().Format("20060102-150405"))
-	dir, err := home.VMDir(cmd, name)
-	if err != nil {
-		return err
-	}
-	return withVMLock(cliutil.CommandContext(cmd), dir, func() error {
-		r, err := h.create(cmd, args[0], name)
-		if err != nil {
-			return err
-		}
-		if err := h.launch(cmd, dir, r); err != nil {
-			return errors.Join(err, cleanupFailedVM(cmd, dir, r))
-		}
-		fmt.Printf("%s (pid %d)\n", r.Name, r.PID)
-		return nil
-	})
+	return h.createVM(cmd, args[0], true)
 }
 
 func (h *Handler) Start(cmd *cobra.Command, args []string) error {
@@ -89,7 +62,7 @@ func (h *Handler) Start(cmd *cobra.Command, args []string) error {
 			if err := h.launch(cmd, dir, r); err != nil {
 				return err
 			}
-			unquiesceNet(cmd, r)
+			toggleNet(cmd, r, true)
 			fmt.Printf("%s (pid %d)\n", n, r.PID)
 			return nil
 		}); err != nil {
@@ -116,7 +89,7 @@ func (h *Handler) Stop(cmd *cobra.Command, args []string) error {
 				return err
 			}
 			terminate(ctx, r, grace)
-			quiesceNet(cmd, r)
+			toggleNet(cmd, r, false)
 			stopVNCProxy(ctx, dir)
 			r.PID, r.VNCDisp, r.VNCPass, r.VNCPassSet = 0, -1, "", false // VNC is launch-scoped: gone with the qemu it belonged to
 			return saveRec(dir, r)
@@ -155,10 +128,7 @@ func (h *Handler) RM(cmd *cobra.Command, args []string) error {
 			} else {
 				cleanupCtx, cancel := context.WithTimeout(context.Background(), vmCleanupTimeout)
 				defer cancel()
-				if cleanupErr := procutil.TerminateByCmdline(cleanupCtx, qemuBinary, dir, 0); cleanupErr != nil {
-					return cleanupErr
-				}
-				if cleanupErr := procutil.TerminateByCmdline(cleanupCtx, "qemu-nbd", dir, time.Second); cleanupErr != nil {
+				if cleanupErr := reapStrayHelpers(cleanupCtx, dir); cleanupErr != nil {
 					return cleanupErr
 				}
 			}
@@ -174,6 +144,29 @@ func (h *Handler) RM(cmd *cobra.Command, args []string) error {
 	return nil
 }
 
+func (h *Handler) createVM(cmd *cobra.Command, image string, launch bool) error {
+	name := requestedVMName(cmd, "macos-"+time.Now().Format("20060102-150405"))
+	dir, err := home.VMDir(cmd, name)
+	if err != nil {
+		return err
+	}
+	return withVMLock(cliutil.CommandContext(cmd), dir, func() error {
+		r, err := h.create(cmd, image, name)
+		if err != nil {
+			return err
+		}
+		if !launch {
+			fmt.Println(r.Name)
+			return nil
+		}
+		if err := h.launch(cmd, dir, r); err != nil {
+			return errors.Join(err, cleanupFailedVM(cmd, dir, r))
+		}
+		fmt.Printf("%s (pid %d)\n", r.Name, r.PID)
+		return nil
+	})
+}
+
 func (h *Handler) create(cmd *cobra.Command, image, name string) (r *record, retErr error) {
 	rawDisks, _ := cmd.Flags().GetStringArray("data-disk")
 	diskSpecs, err := parseDataDisks(rawDisks, nil) // fail fast before any scaffolding
@@ -330,10 +323,7 @@ func cleanupFailedVM(cmd *cobra.Command, dir string, r *record) error {
 			errs = append(errs, err)
 		}
 	}
-	if err := procutil.TerminateByCmdline(ctx, qemuBinary, dir, 0); err != nil {
-		errs = append(errs, err)
-	}
-	if err := procutil.TerminateByCmdline(ctx, "qemu-nbd", dir, time.Second); err != nil {
+	if err := reapStrayHelpers(ctx, dir); err != nil {
 		errs = append(errs, err)
 	}
 	if err := os.RemoveAll(dir); err != nil {
@@ -342,6 +332,13 @@ func cleanupFailedVM(cmd *cobra.Command, dir string, r *record) error {
 	return errors.Join(errs...)
 }
 
+func reapStrayHelpers(ctx context.Context, dir string) error {
+	return errors.Join(
+		procutil.TerminateByCmdline(ctx, qemuBinary, vmDirPrefix(dir), 0),
+		procutil.TerminateByCmdline(ctx, "qemu-nbd", vmDirPrefix(dir), time.Second),
+	)
+}
+
 // prepareOpenCore points r.OpenCore at the shared base, or with randomSMBIOS at a per-VM overlay whose config.plist is patched with a unique identity.
 func prepareOpenCore(ctx context.Context, dir, ocBase string, randomSMBIOS bool, r *record) error {
 	if !randomSMBIOS {

cmd/vm/net_linux.go

@@ -74,12 +74,18 @@ func provisionNet(cmd *cobra.Command, r *record) (tap, netns, mac string, err er
 		return "", "", "", fmt.Errorf("prepare network: %w", err)
 	}
 	cfgs, err := provider.Add(ctx, r.VMID, vmCfg, network.AddRange(0, 1)...)
+	if err == nil && len(cfgs) == 0 {
+		err = errors.New("network add returned no NIC")
+	}
 	if err != nil {
+		// Add rolls back only what it created itself; the netns from Prepare is ours to drop
+		rctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), vmCleanupTimeout)
+		defer cancel()
+		if delErr := provider.Delete(rctx, r.VMID); delErr != nil {
+			log.WithFunc("cmd.vm.provisionNet").Warnf(rctx, "rollback network for %s: %v", r.VMID, delErr)
+		}
 		return "", "", "", fmt.Errorf("add network: %w", err)
 	}
-	if len(cfgs) == 0 {
-		return "", "", "", errors.New("network add returned no NIC")
-	}
 	mac = cfgs[0].MAC
 	if r.NetMode != netCNI {
 		mac = cmp.Or(r.MAC, mac)
@@ -108,39 +114,29 @@ func teardownNet(ctx context.Context, cmd *cobra.Command, r *record) error {
 	if err := provider.Quiesce(ctx, r.VMID); err != nil {
 		logger.Warnf(ctx, "quiesce network for %s: %v", r.VMID, err)
 	}
-	if _, err := provider.Delete(ctx, []string{r.VMID}); err != nil {
+	if err := provider.Delete(ctx, r.VMID); err != nil {
 		return fmt.Errorf("teardown network for %s: %w", r.VMID, err)
 	}
 	return nil
 }
 
-// quiesceNet downs a stopped VM's owned NICs so a dead VMM's carrier-less TAP can't storm host softirqs via the tc mirred redirect; unquiesceNet reverses it on start.
-func quiesceNet(cmd *cobra.Command, r *record) {
+// toggleNet downs a stopped VM's owned NICs so a dead VMM's carrier-less TAP can't storm host softirqs via the tc mirred redirect, and brings them back up on start.
+func toggleNet(cmd *cobra.Command, r *record, up bool) {
 	if !r.TapOwned {
 		return
 	}
 	ctx := cliutil.CommandContext(cmd)
-	logger := log.WithFunc("cmd.vm.quiesceNet")
-	if provider, err := newProvider(cmd, r); err != nil {
-		logger.Warnf(ctx, "quiesce network for %s: %v", r.VMID, err)
-	} else if err := provider.Quiesce(ctx, r.VMID); err != nil {
-		logger.Warnf(ctx, "quiesce network for %s: %v", r.VMID, err)
+	logger := log.WithFunc("cmd.vm.toggleNet")
+	verb, toggle := "quiesce", network.Network.Quiesce
+	if up {
+		verb, toggle = "unquiesce", network.Network.Unquiesce
 	}
-	setTapLink(ctx, r, false)
-}
-
-func unquiesceNet(cmd *cobra.Command, r *record) {
-	if !r.TapOwned {
-		return
-	}
-	ctx := cliutil.CommandContext(cmd)
-	logger := log.WithFunc("cmd.vm.unquiesceNet")
 	if provider, err := newProvider(cmd, r); err != nil {
-		logger.Warnf(ctx, "unquiesce network for %s: %v", r.VMID, err)
-	} else if err := provider.Unquiesce(ctx, r.VMID); err != nil {
-		logger.Warnf(ctx, "unquiesce network for %s: %v", r.VMID, err)
+		logger.Warnf(ctx, "%s network for %s: %v", verb, r.VMID, err)
+	} else if err := toggle(provider, ctx, r.VMID); err != nil {
+		logger.Warnf(ctx, "%s network for %s: %v", verb, r.VMID, err)
 	}
-	setTapLink(ctx, r, true)
+	setTapLink(ctx, r, up)
 }
 
 // setTapLink flips a host-netns TAP's admin state: cocoon's bridge backend no-ops Quiesce, so the toggle lives here; a CNI TAP is inside a netns and is the provider's job.

cmd/vm/net_other.go

@@ -17,9 +17,7 @@ func provisionNet(_ *cobra.Command, _ *record) (tap, netns, mac string, err erro
 
 func teardownNet(_ context.Context, _ *cobra.Command, _ *record) error { return nil }
 
-func quiesceNet(_ *cobra.Command, _ *record) {}
-
-func unquiesceNet(_ *cobra.Command, _ *record) {}
+func toggleNet(_ *cobra.Command, _ *record, _ bool) {}
 
 func launchCmd(_ *record, args []string) *exec.Cmd {
 	return exec.Command(qemuBinary, args...)

cmd/vm/query.go

@@ -28,9 +28,9 @@ func (h *Handler) List(cmd *cobra.Command, _ []string) error {
 		}
 	}
 	return cliutil.OutputFormatted(cmd, recs, func(w *tabwriter.Writer) {
-		fmt.Fprintln(w, "NAME\tSTATE\tCPU\tMEM\tNET\tVNC\tSSH\tIMAGE\tCREATED") //nolint:errcheck
+		fmt.Fprintln(w, "NAME\tSTATE\tCPU\tMEM\tNET\tVNC\tSSH\tIMAGE\tCREATED") //nolint:errcheck // the tabwriter flush reports the write error
 		for _, r := range recs {
-			fmt.Fprintf(w, "%s\t%s\t%d\t%sM\t%s\t%s\t%s\t%s\t%s\n", //nolint:errcheck
+			fmt.Fprintf(w, "%s\t%s\t%d\t%sM\t%s\t%s\t%s\t%s\t%s\n", //nolint:errcheck // the tabwriter flush reports the write error
 				r.Name, vmState(r), r.CPUs, r.Memory, cmp.Or(r.NetMode, netUser),
 				vncCol(r), sshCol(r), r.Image, formatTime(r.Created))
 		}