podman machine init: do not write config unlocked

First make sure we check that a given VM exist when holding the VM lock
for it. The check in cmd/podman/machine/init.go is a nice quick out but
not enough to ensure that 2 processes to not create the same VM at the
same time. The only way to ensure this is by holding the lock and
checking if the VM config file exists.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

cmd/podman/machine/init.go

@@ -208,16 +208,11 @@ func initMachine(cmd *cobra.Command, args []string) error {
 	// 	return err
 	// }
 
-	mc, err := shim.Init(initOpts, provider)
+	err = shim.Init(initOpts, provider)
 	if err != nil {
 		return err
 	}
 
-	// TODO callback needed for the configuration file
-	if err := mc.Write(); err != nil {
-		return err
-	}
-
 	newMachineEvent(events.Init, events.Event{Name: initOpts.Name})
 	fmt.Println("Machine init complete")
 

pkg/machine/shim/host.go

@@ -15,6 +15,7 @@ import (
 	machineDefine "github.com/containers/podman/v5/pkg/machine/define"
 	"github.com/containers/podman/v5/pkg/machine/env"
 	"github.com/containers/podman/v5/pkg/machine/ignition"
+	"github.com/containers/podman/v5/pkg/machine/lock"
 	"github.com/containers/podman/v5/pkg/machine/proxyenv"
 	"github.com/containers/podman/v5/pkg/machine/vmconfigs"
 	"github.com/containers/podman/v5/utils"
@@ -66,7 +67,7 @@ func List(vmstubbers []vmconfigs.VMProvider, _ machine.ListOptions) ([]*machine.
 	return lrs, nil
 }
 
-func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.MachineConfig, error) {
+func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) error {
 	var (
 		err            error
 		imageExtension string
@@ -79,21 +80,28 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	dirs, err := env.GetMachineDirs(mp.VMType())
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	sshIdentityPath, err := env.GetSSHIdentityPath(machineDefine.DefaultIdentityName)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	sshKey, err := machine.GetSSHKeys(sshIdentityPath)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	mc, err := vmconfigs.NewMachineConfig(opts, dirs, sshIdentityPath, mp.VMType())
+	machineLock, err := lock.GetMachineLock(opts.Name, dirs.ConfigDir.GetPath())
 	if err != nil {
-		return nil, err
+		return err
+	}
+	machineLock.Lock()
+	defer machineLock.Unlock()
+
+	mc, err := vmconfigs.NewMachineConfig(opts, dirs, sshIdentityPath, mp.VMType(), machineLock)
+	if err != nil {
+		return err
 	}
 
 	mc.Version = vmconfigs.MachineConfigVersion
@@ -128,7 +136,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	imagePath, err = dirs.DataDir.AppendToNewVMFile(fmt.Sprintf("%s-%s%s", opts.Name, runtime.GOARCH, imageExtension), nil)
 	if err != nil {
-		return nil, err
+		return err
 	}
 	mc.ImagePath = imagePath
 
@@ -142,7 +150,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 	// "docker://quay.io/something/someManifest
 
 	if err := mp.GetDisk(opts.Image, dirs, mc); err != nil {
-		return nil, err
+		return err
 	}
 
 	callbackFuncs.Add(mc.ImagePath.Delete)
@@ -151,7 +159,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	ignitionFile, err := mc.IgnitionFile()
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	uid := os.Getuid()
@@ -184,22 +192,22 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 	// copy it into the conf dir
 	if len(opts.IgnitionPath) > 0 {
 		err = ignBuilder.BuildWithIgnitionFile(opts.IgnitionPath)
-		return nil, err
+		return err
 	}
 
 	err = ignBuilder.GenerateIgnitionConfig()
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	readyIgnOpts, err := mp.PrepareIgnition(mc, &ignBuilder)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	readyUnitFile, err := ignition.CreateReadyUnitFile(mp.VMType(), readyIgnOpts)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	readyUnit := ignition.Unit{
@@ -216,7 +224,7 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	// TODO AddSSHConnectionToPodmanSocket could take an machineconfig instead
 	if err := connection.AddSSHConnectionsToPodmanSocket(mc.HostUser.UID, mc.SSH.Port, mc.SSH.IdentityPath, mc.Name, mc.SSH.RemoteUsername, opts); err != nil {
-		return nil, err
+		return err
 	}
 
 	cleanup := func() error {
@@ -226,15 +234,15 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
 
 	err = mp.CreateVM(createOpts, mc, &ignBuilder)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	err = ignBuilder.Build()
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	return mc, err
+	return mc.Write()
 }
 
 // VMExists looks across given providers for a machine's existence.  returns the actual config and found bool

pkg/machine/vmconfigs/machine.go

@@ -18,6 +18,7 @@ import (
 	"github.com/containers/podman/v5/pkg/machine/lock"
 	"github.com/containers/podman/v5/pkg/machine/ports"
 	"github.com/containers/storage/pkg/ioutils"
+	"github.com/containers/storage/pkg/lockfile"
 	"github.com/sirupsen/logrus"
 )
 
@@ -42,16 +43,11 @@ var (
 
 type RemoteConnectionType string
 
-// NewMachineConfig creates the initial machine configuration file from cli options
-func NewMachineConfig(opts define.InitOptions, dirs *define.MachineDirs, sshIdentityPath string, vmtype define.VMType) (*MachineConfig, error) {
+// NewMachineConfig creates the initial machine configuration file from cli options.
+func NewMachineConfig(opts define.InitOptions, dirs *define.MachineDirs, sshIdentityPath string, vmtype define.VMType, machineLock *lockfile.LockFile) (*MachineConfig, error) {
 	mc := new(MachineConfig)
 	mc.Name = opts.Name
 	mc.dirs = dirs
-
-	machineLock, err := lock.GetMachineLock(opts.Name, dirs.ConfigDir.GetPath())
-	if err != nil {
-		return nil, err
-	}
 	mc.lock = machineLock
 
 	// Assign Dirs
@@ -60,6 +56,11 @@ func NewMachineConfig(opts define.InitOptions, dirs *define.MachineDirs, sshIden
 		return nil, err
 	}
 	mc.configPath = cf
+	// Given that we are locked now and check again that the config file does not exists,
+	// if it does it means the VM was already created and we should error.
+	if _, err := os.Stat(cf.Path); err == nil {
+		return nil, fmt.Errorf("%s: %w", opts.Name, define.ErrVMAlreadyExists)
+	}
 
 	if vmtype != define.QemuVirt && len(opts.USBs) > 0 {
 		return nil, fmt.Errorf("USB host passthrough not supported for %s machines", vmtype)