Merge pull request #3923 from AkihiroSuda/fix-3919

driver: remove AcceptConfig

Author: AkihiroSuda

pkg/driver/driver.go

@@ -79,10 +79,13 @@ type Driver interface {
 	Info() Info
 
 	// Configure sets the configuration for the instance.
+	// TODO: merge Configure and FillConfig?
+	// Or come up with a better name to clarify the difference.
 	Configure(inst *limatype.Instance) *ConfiguredDriver
 
-	AcceptConfig(cfg *limatype.LimaYAML, filePath string) error
-	FillConfig(cfg *limatype.LimaYAML, filePath string) error
+	// FillConfig fills and validates the configuration for the instance.
+	// The config is not set to the instance.
+	FillConfig(ctx context.Context, cfg *limatype.LimaYAML, filePath string) error
 
 	SSHAddress(ctx context.Context) (string, error)
 }

pkg/driver/external/client/methods.go

@@ -115,11 +115,7 @@ func (d *DriverClient) Delete(ctx context.Context) error {
 	return nil
 }
 
-func (d *DriverClient) AcceptConfig(_ *limatype.LimaYAML, _ string) error {
-	return errors.New("pre-configured driver action not implemented in client driver")
-}
-
-func (d *DriverClient) FillConfig(_ *limatype.LimaYAML, _ string) error {
+func (d *DriverClient) FillConfig(_ context.Context, _ *limatype.LimaYAML, _ string) error {
 	return errors.New("pre-configured driver action not implemented in client driver")
 }
 

pkg/driver/external/server/server.go

@@ -57,7 +57,7 @@ func Serve(ctx context.Context, driver driver.Driver) {
 	inspectStatus := flag.Bool("inspect-status", false, "Inspect status of the driver")
 	flag.Parse()
 	if *preConfiguredDriverAction {
-		handlePreConfiguredDriverAction(driver)
+		handlePreConfiguredDriverAction(ctx, driver)
 		return
 	}
 	if *inspectStatus {
@@ -188,7 +188,7 @@ func handleInspectStatus(driver driver.Driver) {
 	}
 }
 
-func handlePreConfiguredDriverAction(driver driver.Driver) {
+func handlePreConfiguredDriverAction(ctx context.Context, driver driver.Driver) {
 	decoder := json.NewDecoder(os.Stdin)
 	encoder := json.NewEncoder(os.Stdout)
 
@@ -198,11 +198,7 @@ func handlePreConfiguredDriverAction(driver driver.Driver) {
 	}
 
 	config := &payload.Config
-	if err := driver.AcceptConfig(config, payload.FilePath); err != nil {
-		fmt.Fprintf(os.Stderr, "Failed to accept config: %v", err)
-	}
-
-	if err := driver.FillConfig(config, payload.FilePath); err != nil {
+	if err := driver.FillConfig(ctx, config, payload.FilePath); err != nil {
 		fmt.Fprintf(os.Stderr, "Failed to fill config: %v", err)
 	}
 

pkg/driver/qemu/qemu_driver.go

@@ -77,26 +77,37 @@ func (l *LimaQemuDriver) Configure(inst *limatype.Instance) *driver.ConfiguredDr
 }
 
 func (l *LimaQemuDriver) Validate(ctx context.Context) error {
+	return validateConfig(ctx, l.Instance.Config)
+}
+
+func validateConfig(ctx context.Context, cfg *limatype.LimaYAML) error {
+	if cfg == nil {
+		return errors.New("configuration is nil")
+	}
 	if runtime.GOOS == "darwin" {
-		if err := l.checkBinarySignature(ctx); err != nil {
+		var vmArch string
+		if cfg.Arch != nil {
+			vmArch = *cfg.Arch
+		}
+		if err := checkBinarySignature(ctx, vmArch); err != nil {
 			return err
 		}
 	}
-	if err := l.validateMountType(); err != nil {
+	if err := validateMountType(cfg); err != nil {
 		return err
 	}
 
+	if cfg.VMOpts.QEMU.MinimumVersion != nil {
+		if _, err := semver.NewVersion(*cfg.VMOpts.QEMU.MinimumVersion); err != nil {
+			return fmt.Errorf("field `vmOpts.qemu.minimumVersion` must be a semvar value, got %q: %w", *cfg.VMOpts.QEMU.MinimumVersion, err)
+		}
+	}
+
 	return nil
 }
 
 // Helper method for mount type validation.
-func (l *LimaQemuDriver) validateMountType() error {
-	if l.Instance == nil || l.Instance.Config == nil {
-		return errors.New("instance configuration is not set")
-	}
-
-	cfg := l.Instance.Config
-
+func validateMountType(cfg *limatype.LimaYAML) error {
 	if cfg.MountType != nil && *cfg.MountType == limatype.VIRTIOFS && runtime.GOOS != "linux" {
 		return fmt.Errorf("field `mountType` must be %q or %q for QEMU driver on non-Linux, got %q",
 			limatype.REVSSHFS, limatype.NINEP, *cfg.MountType)
@@ -111,7 +122,7 @@ func (l *LimaQemuDriver) validateMountType() error {
 	return nil
 }
 
-func (l *LimaQemuDriver) FillConfig(cfg *limatype.LimaYAML, filePath string) error {
+func (l *LimaQemuDriver) FillConfig(ctx context.Context, cfg *limatype.LimaYAML, filePath string) error {
 	if cfg.VMType == nil {
 		cfg.VMType = ptr.Of(limatype.QEMU)
 	}
@@ -174,57 +185,7 @@ func (l *LimaQemuDriver) FillConfig(cfg *limatype.LimaYAML, filePath string) err
 		return fmt.Errorf("mount type %q is explicitly unsupported", *cfg.MountType)
 	}
 
-	return nil
-}
-
-func (l *LimaQemuDriver) AcceptConfig(cfg *limatype.LimaYAML, _ string) error {
-	if l.Instance == nil {
-		l.Instance = &limatype.Instance{}
-	}
-	l.Instance.Config = cfg
-
-	if err := l.Validate(context.Background()); err != nil {
-		return fmt.Errorf("config not supported by the QEMU driver: %w", err)
-	}
-
-	if cfg.VMOpts.QEMU.MinimumVersion != nil {
-		if _, err := semver.NewVersion(*cfg.VMOpts.QEMU.MinimumVersion); err != nil {
-			return fmt.Errorf("field `vmOpts.qemu.minimumVersion` must be a semvar value, got %q: %w", *cfg.VMOpts.QEMU.MinimumVersion, err)
-		}
-	}
-
-	if runtime.GOOS == "darwin" {
-		if cfg.Arch != nil && limayaml.IsNativeArch(*cfg.Arch) {
-			logrus.Debugf("ResolveVMType: resolved VMType %q (non-native arch=%q is specified in []*LimaYAML{o,y,d})", "qemu", *cfg.Arch)
-			return nil
-		}
-		if limayaml.ResolveArch(cfg.Arch) == limatype.X8664 && cfg.Firmware.LegacyBIOS != nil && *cfg.Firmware.LegacyBIOS {
-			logrus.Debugf("ResolveVMType: resolved VMType %q (firmware.legacyBIOS is specified in []*LimaYAML{o,y,d} on x86_64)", "qemu")
-			return nil
-		}
-		if cfg.MountType != nil && *cfg.MountType == limatype.NINEP {
-			logrus.Debugf("ResolveVMType: resolved VMType %q (mountType=%q is specified in []*LimaYAML{o,y,d})", "qemu", limatype.NINEP)
-			return nil
-		}
-		if cfg.Audio.Device != nil {
-			switch *cfg.Audio.Device {
-			case "", "none", "default", "vz":
-				// NOP
-			default:
-				logrus.Debugf("ResolveVMType: resolved VMType %q (audio.device=%q is specified in []*LimaYAML{o,y,d})", "qemu", *cfg.Audio.Device)
-				return nil
-			}
-		}
-		if cfg.Video.Display != nil {
-			display := *cfg.Video.Display
-			if display != "" && display != "none" && display != "default" && display != "vz" {
-				logrus.Debugf("ResolveVMType: resolved VMType %q (video.display=%q is specified in []*LimaYAML{o,y,d})", "qemu", display)
-				return nil
-			}
-		}
-	}
-
-	return nil
+	return validateConfig(ctx, cfg)
 }
 
 func (l *LimaQemuDriver) BootScripts() (map[string][]byte, error) {
@@ -414,18 +375,18 @@ func waitFileExists(path string, timeout time.Duration) error {
 
 // Ask the user to sign the qemu binary with the "com.apple.security.hypervisor" if needed.
 // Workaround for https://github.com/lima-vm/lima/issues/1742
-func (l *LimaQemuDriver) checkBinarySignature(ctx context.Context) error {
+func checkBinarySignature(ctx context.Context, vmArch string) error {
 	macOSProductVersion, err := osutil.ProductVersion()
 	if err != nil {
 		return err
 	}
 	// The codesign --xml option is only available on macOS Monterey and later
-	if !macOSProductVersion.LessThan(*semver.New("12.0.0")) && l.Instance.Arch != "" {
-		qExe, _, err := Exe(l.Instance.Arch)
+	if !macOSProductVersion.LessThan(*semver.New("12.0.0")) && vmArch != "" {
+		qExe, _, err := Exe(vmArch)
 		if err != nil {
-			return fmt.Errorf("failed to find the QEMU binary for the architecture %q: %w", l.Instance.Arch, err)
+			return fmt.Errorf("failed to find the QEMU binary for the architecture %q: %w", vmArch, err)
 		}
-		if accel := Accel(l.Instance.Arch); accel == "hvf" {
+		if accel := Accel(vmArch); accel == "hvf" {
 			entitlementutil.AskToSignIfNotSignedProperly(ctx, qExe)
 		}
 	}

pkg/driver/vz/vz_driver_darwin.go

@@ -11,7 +11,6 @@ import (
 	"errors"
 	"fmt"
 	"net"
-	"os"
 	"path/filepath"
 	"runtime"
 	"time"
@@ -22,7 +21,6 @@ import (
 
 	"github.com/lima-vm/lima/v2/pkg/driver"
 	"github.com/lima-vm/lima/v2/pkg/limatype"
-	"github.com/lima-vm/lima/v2/pkg/limatype/filenames"
 	"github.com/lima-vm/lima/v2/pkg/limayaml"
 	"github.com/lima-vm/lima/v2/pkg/osutil"
 	"github.com/lima-vm/lima/v2/pkg/ptr"
@@ -113,7 +111,7 @@ func (l *LimaVzDriver) Configure(inst *limatype.Instance) *driver.ConfiguredDriv
 	}
 }
 
-func (l *LimaVzDriver) FillConfig(cfg *limatype.LimaYAML, _ string) error {
+func (l *LimaVzDriver) FillConfig(ctx context.Context, cfg *limatype.LimaYAML, _ string) error {
 	if cfg.VMType == nil {
 		cfg.VMType = ptr.Of(limatype.VZ)
 	}
@@ -139,7 +137,7 @@ func (l *LimaVzDriver) FillConfig(cfg *limatype.LimaYAML, _ string) error {
 		cfg.VMOpts.VZ.Rosetta.BinFmt = ptr.Of(false)
 	}
 
-	return nil
+	return validateConfig(ctx, cfg)
 }
 
 func isEmpty(r limatype.Rosetta) bool {
@@ -171,48 +169,14 @@ func (l *LimaVzDriver) BootScripts() (map[string][]byte, error) {
 	return scripts, nil
 }
 
-func (l *LimaVzDriver) AcceptConfig(cfg *limatype.LimaYAML, filePath string) error {
-	if dir, basename := filepath.Split(filePath); dir != "" && basename == filenames.LimaYAML && limayaml.IsExistingInstanceDir(dir) {
-		vzIdentifier := filepath.Join(dir, filenames.VzIdentifier) // since Lima v0.14
-		if _, err := os.Lstat(vzIdentifier); !errors.Is(err, os.ErrNotExist) {
-			logrus.Debugf("ResolveVMType: resolved VMType %q (existing instance, with %q)", "vz", vzIdentifier)
-		}
-	}
-
-	for i, nw := range cfg.Networks {
-		field := fmt.Sprintf("networks[%d]", i)
-		switch {
-		case nw.Lima != "":
-			if nw.VZNAT != nil && *nw.VZNAT {
-				return fmt.Errorf("field `%s.lima` and field `%s.vzNAT` are mutually exclusive", field, field)
-			}
-		case nw.Socket != "":
-			if nw.VZNAT != nil && *nw.VZNAT {
-				return fmt.Errorf("field `%s.socket` and field `%s.vzNAT` are mutually exclusive", field, field)
-			}
-		case nw.VZNAT != nil && *nw.VZNAT:
-			if nw.Lima != "" {
-				return fmt.Errorf("field `%s.vzNAT` and field `%s.lima` are mutually exclusive", field, field)
-			}
-			if nw.Socket != "" {
-				return fmt.Errorf("field `%s.vzNAT` and field `%s.socket` are mutually exclusive", field, field)
-			}
-		}
-	}
-
-	if l.Instance == nil {
-		l.Instance = &limatype.Instance{}
-	}
-	l.Instance.Config = cfg
-
-	if err := l.Validate(context.Background()); err != nil {
-		return fmt.Errorf("config not supported by the VZ driver: %w", err)
-	}
-
-	return nil
+func (l *LimaVzDriver) Validate(ctx context.Context) error {
+	return validateConfig(ctx, l.Instance.Config)
 }
 
-func (l *LimaVzDriver) Validate(_ context.Context) error {
+func validateConfig(_ context.Context, cfg *limatype.LimaYAML) error {
+	if cfg == nil {
+		return errors.New("configuration is nil")
+	}
 	macOSProductVersion, err := osutil.ProductVersion()
 	if err != nil {
 		return err
@@ -223,67 +187,86 @@ func (l *LimaVzDriver) Validate(_ context.Context) error {
 	if runtime.GOARCH == "amd64" && macOSProductVersion.LessThan(*semver.New("15.5.0")) {
 		logrus.Warnf("vmType %s: On Intel Mac, macOS 15.5 or later is required to run Linux 6.12 or later. "+
 			"Update macOS, or change vmType to \"qemu\" if the VM does not start up. (https://github.com/lima-vm/lima/issues/3334)",
-			*l.Instance.Config.VMType)
+			*cfg.VMType)
 	}
-	if l.Instance.Config.MountType != nil && *l.Instance.Config.MountType == limatype.NINEP {
-		return fmt.Errorf("field `mountType` must be %q or %q for VZ driver , got %q", limatype.REVSSHFS, limatype.VIRTIOFS, *l.Instance.Config.MountType)
+	if cfg.MountType != nil && *cfg.MountType == limatype.NINEP {
+		return fmt.Errorf("field `mountType` must be %q or %q for VZ driver , got %q", limatype.REVSSHFS, limatype.VIRTIOFS, *cfg.MountType)
 	}
-	if *l.Instance.Config.Firmware.LegacyBIOS {
-		logrus.Warnf("vmType %s: ignoring `firmware.legacyBIOS`", *l.Instance.Config.VMType)
+	if *cfg.Firmware.LegacyBIOS {
+		logrus.Warnf("vmType %s: ignoring `firmware.legacyBIOS`", *cfg.VMType)
 	}
-	for _, f := range l.Instance.Config.Firmware.Images {
+	for _, f := range cfg.Firmware.Images {
 		switch f.VMType {
 		case "", limatype.VZ:
-			if f.Arch == *l.Instance.Config.Arch {
+			if f.Arch == *cfg.Arch {
 				return errors.New("`firmware.images` configuration is not supported for VZ driver")
 			}
 		}
 	}
-	if unknown := reflectutil.UnknownNonEmptyFields(l.Instance.Config, knownYamlProperties...); l.Instance.Config.VMType != nil && len(unknown) > 0 {
-		logrus.Warnf("vmType %s: ignoring %+v", *l.Instance.Config.VMType, unknown)
+	if unknown := reflectutil.UnknownNonEmptyFields(cfg, knownYamlProperties...); cfg.VMType != nil && len(unknown) > 0 {
+		logrus.Warnf("vmType %s: ignoring %+v", *cfg.VMType, unknown)
 	}
 
-	if !limayaml.IsNativeArch(*l.Instance.Config.Arch) {
-		return fmt.Errorf("unsupported arch: %q", *l.Instance.Config.Arch)
+	if !limayaml.IsNativeArch(*cfg.Arch) {
+		return fmt.Errorf("unsupported arch: %q", *cfg.Arch)
 	}
 
-	for i, image := range l.Instance.Config.Images {
+	for i, image := range cfg.Images {
 		if unknown := reflectutil.UnknownNonEmptyFields(image, "File", "Kernel", "Initrd"); len(unknown) > 0 {
-			logrus.Warnf("vmType %s: ignoring images[%d]: %+v", *l.Instance.Config.VMType, i, unknown)
+			logrus.Warnf("vmType %s: ignoring images[%d]: %+v", *cfg.VMType, i, unknown)
 		}
 	}
 
-	for i, mount := range l.Instance.Config.Mounts {
+	for i, mount := range cfg.Mounts {
 		if unknown := reflectutil.UnknownNonEmptyFields(mount, "Location",
 			"MountPoint",
 			"Writable",
 			"SSHFS",
 			"NineP",
 		); len(unknown) > 0 {
-			logrus.Warnf("vmType %s: ignoring mounts[%d]: %+v", *l.Instance.Config.VMType, i, unknown)
+			logrus.Warnf("vmType %s: ignoring mounts[%d]: %+v", *cfg.VMType, i, unknown)
 		}
 	}
 
-	for i, network := range l.Instance.Config.Networks {
-		if unknown := reflectutil.UnknownNonEmptyFields(network, "VZNAT",
+	for i, nw := range cfg.Networks {
+		if unknown := reflectutil.UnknownNonEmptyFields(nw, "VZNAT",
 			"Lima",
 			"Socket",
 			"MACAddress",
 			"Metric",
 			"Interface",
 		); len(unknown) > 0 {
-			logrus.Warnf("vmType %s: ignoring networks[%d]: %+v", *l.Instance.Config.VMType, i, unknown)
+			logrus.Warnf("vmType %s: ignoring networks[%d]: %+v", *cfg.VMType, i, unknown)
+		}
+
+		field := fmt.Sprintf("networks[%d]", i)
+		switch {
+		case nw.Lima != "":
+			if nw.VZNAT != nil && *nw.VZNAT {
+				return fmt.Errorf("field `%s.lima` and field `%s.vzNAT` are mutually exclusive", field, field)
+			}
+		case nw.Socket != "":
+			if nw.VZNAT != nil && *nw.VZNAT {
+				return fmt.Errorf("field `%s.socket` and field `%s.vzNAT` are mutually exclusive", field, field)
+			}
+		case nw.VZNAT != nil && *nw.VZNAT:
+			if nw.Lima != "" {
+				return fmt.Errorf("field `%s.vzNAT` and field `%s.lima` are mutually exclusive", field, field)
+			}
+			if nw.Socket != "" {
+				return fmt.Errorf("field `%s.vzNAT` and field `%s.socket` are mutually exclusive", field, field)
+			}
 		}
 	}
 
-	switch audioDevice := *l.Instance.Config.Audio.Device; audioDevice {
+	switch audioDevice := *cfg.Audio.Device; audioDevice {
 	case "":
 	case "vz", "default", "none":
 	default:
 		logrus.Warnf("field `audio.device` must be \"vz\", \"default\", or \"none\" for VZ driver, got %q", audioDevice)
 	}
 
-	switch videoDisplay := *l.Instance.Config.Video.Display; videoDisplay {
+	switch videoDisplay := *cfg.Video.Display; videoDisplay {
 	case "vz", "default", "none":
 	default:
 		logrus.Warnf("field `video.display` must be \"vz\", \"default\", or \"none\" for VZ driver , got %q", videoDisplay)