Merge pull request #148 from AkihiroSuda/fix-146

`vde: defer vnl validation until starting qemu` + `pkg/start: exit when hostagent process exited`

Author: AkihiroSuda

pkg/limayaml/validate.go

@@ -179,31 +179,31 @@ func validateNetwork(yNetwork Network) error {
 		// optionally with vde:// prefix.
 		if !strings.Contains(vde.VNL, "://") || strings.HasPrefix(vde.VNL, "vde://") {
 			vdeSwitch := strings.TrimPrefix(vde.VNL, "vde://")
-			fi, err := os.Stat(vdeSwitch)
-			if err != nil {
-				return fmt.Errorf("field `%s.vnl` %q failed stat: %w", field, vdeSwitch, err)
-			}
-			if fi.IsDir() {
-				/* Switch mode (vdeSwitch is dir, port != 65535) */
-				ctlSocket := filepath.Join(vdeSwitch, "ctl")
-				fi, err = os.Stat(ctlSocket)
-				if err != nil {
-					return fmt.Errorf("field `%s.vnl` control socket %q failed stat: %w", field, ctlSocket, err)
-				}
-				if fi.Mode()&os.ModeSocket == 0 {
-					return fmt.Errorf("field `%s.vnl` file %q is not a UNIX socket", field, ctlSocket)
-				}
-				if vde.SwitchPort == 65535 {
-					return fmt.Errorf("field `%s.vnl` points to a non-PTP switch, so the port number must not be 65535", field)
-				}
+			if fi, err := os.Stat(vdeSwitch); err != nil {
+				// negligible when the instance is stopped
+				logrus.WithError(err).Debugf("field `%s.vnl` %q failed stat", field, vdeSwitch)
 			} else {
-				/* PTP mode (vdeSwitch is socket, port == 65535) */
-				if fi.Mode()&os.ModeSocket == 0 {
-					return fmt.Errorf("field `%s.vnl` %q is not a directory nor a UNIX socket", field, vdeSwitch)
-				}
-				if vde.SwitchPort != 65535 {
-					return fmt.Errorf("field `%s.vnl` points to a PTP (switchless) socket %q, so the port number has to be 65535 (got %d)",
-						field, vdeSwitch, vde.SwitchPort)
+				if fi.IsDir() {
+					/* Switch mode (vdeSwitch is dir, port != 65535) */
+					ctlSocket := filepath.Join(vdeSwitch, "ctl")
+					// ErrNotExist during os.Stat(ctlSocket) can be ignored. ctlSocket does not need to exist until actually starting the VM
+					if fi, err = os.Stat(ctlSocket); err == nil {
+						if fi.Mode()&os.ModeSocket == 0 {
+							return fmt.Errorf("field `%s.vnl` file %q is not a UNIX socket", field, ctlSocket)
+						}
+					}
+					if vde.SwitchPort == 65535 {
+						return fmt.Errorf("field `%s.vnl` points to a non-PTP switch, so the port number must not be 65535", field)
+					}
+				} else {
+					/* PTP mode (vdeSwitch is socket, port == 65535) */
+					if fi.Mode()&os.ModeSocket == 0 {
+						return fmt.Errorf("field `%s.vnl` %q is not a directory nor a UNIX socket", field, vdeSwitch)
+					}
+					if vde.SwitchPort != 65535 {
+						return fmt.Errorf("field `%s.vnl` points to a PTP (switchless) socket %q, so the port number has to be 65535 (got %d)",
+							field, vdeSwitch, vde.SwitchPort)
+					}
 				}
 			}
 		} else if runtime.GOOS != "linux" {

pkg/qemu/qemu.go

@@ -1,8 +1,10 @@
 package qemu
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -136,15 +138,64 @@ func appendArgsIfNoConflict(args []string, k, v string) []string {
 	}
 	return append(args, k, v)
 }
+
+type features struct {
+	// NetdevHelp is the output of `qemu-system-x86_64 -accel help`
+	// e.g. "Accelerators supported in QEMU binary:\ntcg\nhax\nhvf\n"
+	// Not machine-readable, but checking strings.Contains() should be fine.
+	AccelHelp []byte
+	// NetdevHelp is the output of `qemu-system-x86_64 -netdev help`
+	// e.g. "Available netdev backend types:\nsocket\nhubport\ntap\nuser\nvde\nbridge\vhost-user\n"
+	// Not machine-readable, but checking strings.Contains() should be fine.
+	NetdevHelp []byte
+}
+
+func inspectFeatures(exe string) (*features, error) {
+	var (
+		f      features
+		stdout bytes.Buffer
+		stderr bytes.Buffer
+	)
+	cmd := exec.Command(exe, "-M", "none", "-accel", "help")
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		return nil, fmt.Errorf("failed to run %v: stdout=%q, stderr=%q", cmd.Args, stdout.String(), stderr.String())
+	}
+	f.AccelHelp = stdout.Bytes()
+
+	cmd = exec.Command(exe, "-M", "none", "-netdev", "help")
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		return nil, fmt.Errorf("failed to run %v: stdout=%q, stderr=%q", cmd.Args, stdout.String(), stderr.String())
+	}
+	f.NetdevHelp = stdout.Bytes()
+	return &f, nil
+}
+
 func Cmdline(cfg Config) (string, []string, error) {
 	y := cfg.LimaYAML
 	exe, args, err := getExe(y.Arch)
 	if err != nil {
 		return "", nil, err
 	}
 
+	features, err := inspectFeatures(exe)
+	if err != nil {
+		return "", nil, err
+	}
+
 	// Architecture
 	accel := getAccel(y.Arch)
+	if !strings.Contains(string(features.AccelHelp), accel) {
+		errStr := fmt.Sprintf("accelerator %q is not supported by %s", accel, exe)
+		if accel == "hvf" && y.Arch == limayaml.AARCH64 {
+			errStr += " ( Hint: as of August 2021, qemu-system-aarch64 on ARM Mac needs to be patched for enabling hvf accelerator,"
+			errStr += " see https://gist.github.com/nrjdalal/e70249bb5d2e9d844cc203fd11f74c55 )"
+		}
+		return "", nil, errors.New(errStr)
+	}
 	switch y.Arch {
 	case limayaml.X8664:
 		// NOTE: "-cpu host" seems to cause kernel panic
@@ -202,11 +253,29 @@ func Cmdline(cfg Config) (string, []string, error) {
 	args = append(args, "-netdev", fmt.Sprintf("user,id=net0,net=%s,dhcpstart=%s,hostfwd=tcp:127.0.0.1:%d-:22",
 		qemuconst.SlirpNetwork, qemuconst.SlirpIPAddress, y.SSH.LocalPort))
 	args = append(args, "-device", "virtio-net-pci,netdev=net0,mac="+limayaml.MACAddress(cfg.InstanceDir))
+	if len(y.Network.VDE) > 0 && !strings.Contains(string(features.NetdevHelp), "vde") {
+		return "", nil, fmt.Errorf("netdev \"vde\" is not supported by %s ( Hint: recompile QEMU with `configure --enable-vde` )", exe)
+	}
 	for i, vde := range y.Network.VDE {
 		// VDE4 accepts VNL like vde:///var/run/vde.ctl as well as file path like /var/run/vde.ctl .
 		// VDE2 only accepts the latter form.
 		// VDE2 supports macOS but VDE4 does not yet, so we trim vde:// prefix here for VDE2 compatibility.
 		vdeSock := strings.TrimPrefix(vde.VNL, "vde://")
+		if !strings.Contains(vdeSock, "://") {
+			if _, err := os.Stat(vdeSock); err != nil {
+				return "", nil, fmt.Errorf("cannot use VNL %q: %w", vde.VNL, err)
+			}
+			// vdeSock is a directory, unless vde.SwitchPort == 65535 (PTP)
+			actualSocket := filepath.Join(vdeSock, "ctl")
+			if vde.SwitchPort == 65535 { // PTP
+				actualSocket = vdeSock
+			}
+			if st, err := os.Stat(actualSocket); err != nil {
+				return "", nil, fmt.Errorf("cannot use VNL %q: failed to stat %q: %w", vde.VNL, actualSocket, err)
+			} else if st.Mode()&fs.ModeSocket == 0 {
+				return "", nil, fmt.Errorf("cannot use VNL %q: %q is not a socket: %w", vde.VNL, actualSocket, err)
+			}
+		}
 		args = append(args, "-netdev", fmt.Sprintf("vde,id=net%d,sock=%s", i+1, vdeSock))
 		args = append(args, "-device", fmt.Sprintf("virtio-net-pci,netdev=net%d,mac=%s", i+1, vde.MACAddress))
 	}

pkg/start/start.go

@@ -89,8 +89,26 @@ func Start(ctx context.Context, inst *store.Instance) error {
 		return err
 	}
 
-	return watchHostAgentEvents(ctx, inst.Name, haStdoutPath, haStderrPath, begin)
-	// leave the hostagent process running
+	watchErrCh := make(chan error)
+	go func() {
+		watchErrCh <- watchHostAgentEvents(ctx, inst.Name, haStdoutPath, haStderrPath, begin)
+		close(watchErrCh)
+	}()
+	waitErrCh := make(chan error)
+	go func() {
+		waitErrCh <- haCmd.Wait()
+		close(waitErrCh)
+	}()
+
+	select {
+	case watchErr := <-watchErrCh:
+		// watchErr can be nil
+		return watchErr
+		// leave the hostagent process running
+	case waitErr := <-waitErrCh:
+		// waitErr should not be nil
+		return fmt.Errorf("host agent process has exited: %w", waitErr)
+	}
 }
 
 func waitHostAgentStart(ctx context.Context, haPIDPath, haStderrPath string) error {