fix: HMP echo detection and CNI dirs in the VM record (#43)

* fix: treat only the echoed command as HMP echo

A password beginning with a double quote makes QEMU answer 'set_password: string expected' and 'Try "help set_password"'; both lines contain set_password, so the substring test took them for echo and the launch reported a password that was never set. The echo is now the first line carrying the command prefix, and only that line.

* fix: remember the CNI dirs in the VM record so rm can release the NIC

vm rm has no --cni-conf-dir/--cni-bin-dir, so a CNI VM created against a non-default CNI installation kept its NIC forever ('nic release incomplete'). run and clone now persist both dirs and every provisioning verb reads them from the record; records without them keep the defaults.

* fix: match the HMP echo once by substring; rm takes the CNI dirs for older records

QEMU's readline echoes the typed command with redraw sequences, so a prefix test would have taken every successful echo for a rejection. The echo is the first line containing 'set_password ' and only that line. Records written before the CNI dirs were persisted resolve flag, then record, then default, so rm --cni-conf-dir/--cni-bin-dir can release their NICs.

* review: flagOr already carries the fallback; docs name the rm recovery flags

Author: CMGS

cmd/vm/clone.go

@@ -138,6 +138,8 @@ func (h *Handler) clone(cmd *cobra.Command, srcRec *record, name string) (retErr
 	}
 	tapFlag, _ := cmd.Flags().GetString("tap")
 	r.Tap = tapFlag
+	r.CNIConfDir = flagOr(cmd, "cni-conf-dir", srcRec.CNIConfDir)
+	r.CNIBinDir = flagOr(cmd, "cni-bin-dir", srcRec.CNIBinDir)
 	if err = applyNet(cmd, r); err != nil {
 		return err
 	}

cmd/vm/commands.go

@@ -72,6 +72,8 @@ func Command(h *Handler) *cobra.Command {
 		RunE:  h.RM,
 	}
 	rmCmd.Flags().Bool("force", false, "force kill (immediate SIGKILL, skip the ACPI grace window)")
+	rmCmd.Flags().String("cni-conf-dir", "", "CNI config dir for a VM created before the record remembered it")
+	rmCmd.Flags().String("cni-bin-dir", "", "CNI plugin dir for a VM created before the record remembered it")
 
 	snapshotCmd := &cobra.Command{
 		Use:   "snapshot VM",

cmd/vm/handler.go

@@ -53,6 +53,8 @@ type record struct {
 	TapOwned     bool         `json:"tap_owned,omitempty"`  // cocoon auto-created the TAP (tear down on rm); false for user --tap
 	BridgeDev    string       `json:"bridge_dev,omitempty"` // bridge to enslave the TAP to; persisted so rm can tear down without --bridge
 	Netns        string       `json:"netns,omitempty"`      // netns path the qemu process runs in (CNI); "" otherwise
+	CNIConfDir   string       `json:"cni_conf_dir,omitempty"`
+	CNIBinDir    string       `json:"cni_bin_dir,omitempty"`
 
 	PID int `json:"pid"`
 

cmd/vm/lifecycle.go

@@ -207,11 +207,13 @@ func (h *Handler) create(cmd *cobra.Command, image, name string) (r *record, ret
 	tap, _ := cmd.Flags().GetString("tap")
 	huge, _ := cmd.Flags().GetBool("hugepages")
 	exitOnReboot, _ := cmd.Flags().GetBool("exit-on-reboot")
+	cniConfDir, _ := cmd.Flags().GetString("cni-conf-dir")
+	cniBinDir, _ := cmd.Flags().GetString("cni-bin-dir")
 	r = &record{
 		Name: name, Image: image, ImageDigest: digest, Disk: overlay, OVMFCode: code, OVMFVars: ovmfVars,
 		CPUs: cpus, Memory: mem, Storage: storage, VNCDisp: vnc, SSHPort: ssh, VNCPass: vncPass, NetMode: netMode, Tap: tap, Hugepages: huge,
-		ExitOnReboot: exitOnReboot,
-		VMID:         utils.GenerateID(), Created: time.Now().Format(time.RFC3339),
+		ExitOnReboot: exitOnReboot, CNIConfDir: cniConfDir, CNIBinDir: cniBinDir,
+		VMID: utils.GenerateID(), Created: time.Now().Format(time.RFC3339),
 	}
 	if r.DataDisks, err = createDataDisks(ctx, dir, diskSpecs); err != nil {
 		return nil, err

cmd/vm/net_linux.go

@@ -28,19 +28,19 @@ import (
 const netScope = "cm"
 
 // netConf is the cocoon network config: bridge/CNI provisioning shares cocoon's forwarding plane, keyed under our own device family.
-func netConf(cmd *cobra.Command) *config.Config {
+func netConf(cmd *cobra.Command, r *record) *config.Config {
 	return &config.Config{
 		RootDir:    home.Dir(cmd),
 		DNS:        "8.8.8.8,1.1.1.1",
-		CNIConfDir: flagOr(cmd, "cni-conf-dir", "/etc/cni/net.d"),
-		CNIBinDir:  flagOr(cmd, "cni-bin-dir", "/opt/cni/bin"),
+		CNIConfDir: cmp.Or(flagOr(cmd, "cni-conf-dir", ""), r.CNIConfDir, "/etc/cni/net.d"),
+		CNIBinDir:  cmp.Or(flagOr(cmd, "cni-bin-dir", ""), r.CNIBinDir, "/opt/cni/bin"),
 		NetScope:   netScope,
 	}
 }
 
 // newProvider builds the cocoon network provider: tap/bridge both use the bridge backend (QEMU opens the TAP in the host netns, so it must be a host-side bridge port); cni's TAP lives in a netns.
 func newProvider(cmd *cobra.Command, r *record) (network.Network, error) {
-	conf := netConf(cmd)
+	conf := netConf(cmd, r)
 	switch r.NetMode {
 	case netCNI:
 		store, err := metajson.Open(cni.NewConfig(conf).JSONNamespace())
@@ -100,7 +100,7 @@ func teardownNet(ctx context.Context, cmd *cobra.Command, r *record) error {
 	}
 	bridgeMode := r.NetMode == netTAP || r.NetMode == netBridge
 	if bridgeMode {
-		defer bridge.CleanupTAPs(netConf(cmd).BridgeTAPPrefix(), []string{r.VMID})
+		defer bridge.CleanupTAPs(netConf(cmd, r).BridgeTAPPrefix(), []string{r.VMID})
 	}
 	logger := log.WithFunc("cmd.vm.teardownNet")
 	provider, err := newProvider(cmd, r)

cmd/vm/net_linux_test.go

@@ -13,8 +13,26 @@ import (
 	"github.com/cocoonstack/cocoon-macos/home"
 )
 
+func TestNetConfResolvesFlagThenRecordThenDefault(t *testing.T) {
+	cmd := &cobra.Command{}
+	cmd.Flags().String("cni-conf-dir", "", "")
+	cmd.Flags().String("cni-bin-dir", "", "")
+	if got := netConf(cmd, &record{}).CNIConfDir; got != "/etc/cni/net.d" {
+		t.Errorf("default CNIConfDir = %q", got)
+	}
+	if got := netConf(cmd, &record{CNIConfDir: "/rec/net.d", CNIBinDir: "/rec/bin"}).CNIBinDir; got != "/rec/bin" {
+		t.Errorf("record CNIBinDir = %q", got)
+	}
+	if err := cmd.Flags().Set("cni-conf-dir", "/flag/net.d"); err != nil {
+		t.Fatal(err)
+	}
+	if got := netConf(cmd, &record{CNIConfDir: "/rec/net.d"}).CNIConfDir; got != "/flag/net.d" {
+		t.Errorf("flag CNIConfDir = %q, want the flag over the record", got)
+	}
+}
+
 func TestNetConfScope(t *testing.T) {
-	conf := netConf(&cobra.Command{})
+	conf := netConf(&cobra.Command{}, &record{})
 	if got, want := conf.NetScope, "cm"; got != want {
 		t.Errorf("NetScope = %q, want %q", got, want)
 	}

cmd/vm/utils.go

@@ -365,9 +365,14 @@ func setVNCPassword(ctx context.Context, monSock, pw string) error {
 }
 
 func hmpReplied(out string) bool {
+	echoed := false
 	for line := range strings.SplitSeq(out, "\n") {
 		line = strings.TrimSpace(line)
-		if line == "" || strings.HasPrefix(line, strings.TrimSpace(hmpPrompt)) || strings.Contains(line, "set_password") {
+		if line == "" || strings.HasPrefix(line, strings.TrimSpace(hmpPrompt)) {
+			continue
+		}
+		if !echoed && strings.Contains(line, "set_password ") {
+			echoed = true
 			continue
 		}
 		return true

cmd/vm/utils_test.go

@@ -168,6 +168,9 @@ func TestHMPRepliedFlagsAnyMessage(t *testing.T) {
 		{"echo and prompt only", " set_password vnc abcd\r\n(qemu) ", false},
 		{"invalid parameter", " set_password vnc ab cd\r\nError: invalid parameter value: cd\r\n(qemu) ", true},
 		{"display inactive", " set_password vnc abcd\r\nCould not set password\r\n(qemu) ", true},
+		{"unterminated quote", " set_password vnc \"abc\r\nset_password: string expected\r\nTry \"help set_password\" for more information\r\n(qemu) ", true},
+		{"readline redraw echo", "s\x1b[K\x1b[Dse\x1b[K\x1b[D\x1b[Dset_password vnc abcd\r\n(qemu) ", false},
+		{"readline redraw then rejection", "s\x1b[K\x1b[Dset_password vnc \"abc\r\nset_password: string expected\r\n(qemu) ", true},
 	} {
 		if got := hmpReplied(tc.out); got != tc.want {
 			t.Errorf("%s: hmpReplied = %v, want %v", tc.name, got, tc.want)

docs/networking.md

@@ -31,7 +31,10 @@ goes straight to that IP (no port-forward).
 
 `--cni-conf-dir` (default `/etc/cni/net.d`) and `--cni-bin-dir` (default
 `/opt/cni/bin`) point at a non-standard CNI installation; both are ignored by
-the other net modes.
+the other net modes and are remembered in the VM record, so `rm` tears the
+NIC down without repeating them; a clone inherits them from its source. A VM
+created before the record carried them takes `rm --cni-conf-dir` and
+`rm --cni-bin-dir` once.
 
 ## Clones