Merge pull request #3339 from jandubois/no-tilde-mountpoint

AkihiroSuda · web-flow · commit c5eb9acfc22c · 2025-03-13T08:41:14.000+09:00
Do tilde-expansion only once in FillDefaults()
diff --git a/pkg/cidata/cidata.go b/pkg/cidata/cidata.go
@@ -201,14 +201,6 @@ func templateArgs(bootScripts bool, instDir, name string, instConfig *limayaml.L
 	}
 	for i, f := range instConfig.Mounts {
 		tag := fmt.Sprintf("mount%d", i)
-		location, err := localpathutil.Expand(f.Location)
-		if err != nil {
-			return nil, err
-		}
-		mountPoint, err := localpathutil.Expand(*f.MountPoint)
-		if err != nil {
-			return nil, err
-		}
 		options := "defaults"
 		switch fstype {
 		case "9p", "virtiofs":
@@ -221,17 +213,17 @@ func templateArgs(bootScripts bool, instDir, name string, instConfig *limayaml.L
 				options += fmt.Sprintf(",version=%s", *f.NineP.ProtocolVersion)
 				msize, err := units.RAMInBytes(*f.NineP.Msize)
 				if err != nil {
-					return nil, fmt.Errorf("failed to parse msize for %q: %w", location, err)
+					return nil, fmt.Errorf("failed to parse msize for %q: %w", f.Location, err)
 				}
 				options += fmt.Sprintf(",msize=%d", msize)
 				options += fmt.Sprintf(",cache=%s", *f.NineP.Cache)
 			}
 			// don't fail the boot, if virtfs is not available
 			options += ",nofail"
 		}
-		args.Mounts = append(args.Mounts, Mount{Tag: tag, MountPoint: mountPoint, Type: fstype, Options: options})
-		if location == hostHome {
-			args.HostHomeMountPoint = mountPoint
+		args.Mounts = append(args.Mounts, Mount{Tag: tag, MountPoint: *f.MountPoint, Type: fstype, Options: options})
+		if f.Location == hostHome {
+			args.HostHomeMountPoint = *f.MountPoint
 		}
 	}
 
diff --git a/pkg/hostagent/inotify.go b/pkg/hostagent/inotify.go
@@ -11,7 +11,6 @@ import (
 	"strings"
 
 	guestagentapi "github.com/lima-vm/lima/pkg/guestagent/api"
-	"github.com/lima-vm/lima/pkg/localpathutil"
 	"github.com/rjeczalik/notify"
 	"github.com/sirupsen/logrus"
 	"google.golang.org/protobuf/types/known/timestamppb"
@@ -74,20 +73,16 @@ func (a *HostAgent) setupWatchers(events chan notify.EventInfo) error {
 		if !*m.Writable {
 			continue
 		}
-		location, err := localpathutil.Expand(m.Location)
+		symlink, err := filepath.EvalSymlinks(m.Location)
 		if err != nil {
 			return err
 		}
-		symlink, err := filepath.EvalSymlinks(location)
-		if err != nil {
-			return err
-		}
-		if location != symlink {
-			mountSymlinks[symlink] = location
+		if m.Location != symlink {
+			mountSymlinks[symlink] = m.Location
 		}
 
-		logrus.Infof("enable inotify for writable mount: %s", location)
-		err = notify.Watch(path.Join(location, "..."), events, GetNotifyEvent())
+		logrus.Infof("enable inotify for writable mount: %s", m.Location)
+		err = notify.Watch(path.Join(m.Location, "..."), events, GetNotifyEvent())
 		if err != nil {
 			return err
 		}
diff --git a/pkg/hostagent/mount.go b/pkg/hostagent/mount.go
@@ -9,7 +9,6 @@ import (
 	"os"
 
 	"github.com/lima-vm/lima/pkg/limayaml"
-	"github.com/lima-vm/lima/pkg/localpathutil"
 	"github.com/lima-vm/sshocker/pkg/reversesshfs"
 	"github.com/sirupsen/logrus"
 )
@@ -35,16 +34,7 @@ func (a *HostAgent) setupMounts() ([]*mount, error) {
 }
 
 func (a *HostAgent) setupMount(m limayaml.Mount) (*mount, error) {
-	location, err := localpathutil.Expand(m.Location)
-	if err != nil {
-		return nil, err
-	}
-
-	mountPoint, err := localpathutil.Expand(*m.MountPoint)
-	if err != nil {
-		return nil, err
-	}
-	if err := os.MkdirAll(location, 0o755); err != nil {
+	if err := os.MkdirAll(m.Location, 0o755); err != nil {
 		return nil, err
 	}
 	// NOTE: allow_other requires "user_allow_other" in /etc/fuse.conf
@@ -55,35 +45,35 @@ func (a *HostAgent) setupMount(m limayaml.Mount) (*mount, error) {
 	if *m.SSHFS.FollowSymlinks {
 		sshfsOptions += ",follow_symlinks"
 	}
-	logrus.Infof("Mounting %q on %q", location, mountPoint)
+	logrus.Infof("Mounting %q on %q", m.Location, *m.MountPoint)
 
 	rsf := &reversesshfs.ReverseSSHFS{
 		Driver:              *m.SSHFS.SFTPDriver,
 		SSHConfig:           a.sshConfig,
-		LocalPath:           location,
+		LocalPath:           m.Location,
 		Host:                "127.0.0.1",
 		Port:                a.sshLocalPort,
-		RemotePath:          mountPoint,
+		RemotePath:          *m.MountPoint,
 		Readonly:            !(*m.Writable),
 		SSHFSAdditionalArgs: []string{"-o", sshfsOptions},
 	}
 	if err := rsf.Prepare(); err != nil {
-		return nil, fmt.Errorf("failed to prepare reverse sshfs for %q on %q: %w", location, mountPoint, err)
+		return nil, fmt.Errorf("failed to prepare reverse sshfs for %q on %q: %w", m.Location, *m.MountPoint, err)
 	}
 	if err := rsf.Start(); err != nil {
-		logrus.WithError(err).Warnf("failed to mount reverse sshfs for %q on %q, retrying with `-o nonempty`", location, mountPoint)
+		logrus.WithError(err).Warnf("failed to mount reverse sshfs for %q on %q, retrying with `-o nonempty`", m.Location, *m.MountPoint)
 		// NOTE: nonempty is not supported for libfuse3: https://github.com/canonical/multipass/issues/1381
 		rsf.SSHFSAdditionalArgs = []string{"-o", "nonempty"}
 		if err := rsf.Start(); err != nil {
-			return nil, fmt.Errorf("failed to mount reverse sshfs for %q on %q: %w", location, mountPoint, err)
+			return nil, fmt.Errorf("failed to mount reverse sshfs for %q on %q: %w", m.Location, *m.MountPoint, err)
 		}
 	}
 
 	res := &mount{
 		close: func() error {
-			logrus.Infof("Unmounting %q", location)
-			if closeErr := rsf.Close(); closeErr != nil {
-				return fmt.Errorf("failed to unmount reverse sshfs for %q on %q: %w", location, mountPoint, err)
+			logrus.Infof("Unmounting %q", m.Location)
+			if err := rsf.Close(); err != nil {
+				return fmt.Errorf("failed to unmount reverse sshfs for %q on %q: %w", m.Location, *m.MountPoint, err)
 			}
 			return nil
 		},
diff --git a/pkg/limayaml/defaults.go b/pkg/limayaml/defaults.go
@@ -29,6 +29,7 @@ import (
 	"golang.org/x/sys/cpu"
 
 	"github.com/lima-vm/lima/pkg/identifierutil"
+	"github.com/lima-vm/lima/pkg/localpathutil"
 	. "github.com/lima-vm/lima/pkg/must"
 	"github.com/lima-vm/lima/pkg/networks"
 	"github.com/lima-vm/lima/pkg/osutil"
@@ -822,8 +823,13 @@ func FillDefault(y, d, o *LimaYAML, filePath string, warn bool) {
 				mounts[i].NineP.Cache = ptr.Of(Default9pCacheForRO)
 			}
 		}
+		if location, err := localpathutil.Expand(mount.Location); err == nil {
+			mounts[i].Location = location
+		} else {
+			logrus.WithError(err).Warnf("Couldn't expand location %q", mount.Location)
+		}
 		if mount.MountPoint == nil {
-			mounts[i].MountPoint = ptr.Of(mount.Location)
+			mounts[i].MountPoint = ptr.Of(mounts[i].Location)
 		}
 	}
 
diff --git a/pkg/limayaml/defaults_test.go b/pkg/limayaml/defaults_test.go
@@ -146,7 +146,9 @@ func TestFillDefault(t *testing.T) {
 			},
 		},
 		Mounts: []Mount{
-			{Location: "/tmp"},
+			// Location will be passed through localpathutil.Expand() which will normalize the name
+			// (add a drive letter). So we must start with a valid local path to match it again later.
+			{Location: filepath.Clean(os.TempDir())},
 			{Location: "{{.Dir}}/{{.Param.ONE}}", MountPoint: ptr.Of("/mnt/{{.Param.ONE}}")},
 		},
 		MountType: ptr.Of(NINEP),
@@ -231,7 +233,7 @@ func TestFillDefault(t *testing.T) {
 	expect.Mounts[0].NineP.Cache = ptr.Of(Default9pCacheForRO)
 	expect.Mounts[0].Virtiofs.QueueSize = nil
 	// Only missing Mounts field is Writable, and the default value is also the null value: false
-	expect.Mounts[1].Location = fmt.Sprintf("%s/%s", instDir, y.Param["ONE"])
+	expect.Mounts[1].Location = filepath.Join(instDir, y.Param["ONE"])
 	expect.Mounts[1].MountPoint = ptr.Of(fmt.Sprintf("/mnt/%s", y.Param["ONE"]))
 	expect.Mounts[1].Writable = ptr.Of(false)
 	expect.Mounts[1].SSHFS.Cache = ptr.Of(true)
@@ -321,6 +323,9 @@ func TestFillDefault(t *testing.T) {
 	// User-provided defaults should override any builtin defaults
 
 	// Choose values that are different from the "builtin" defaults
+
+	// Calling filepath.Abs() to add a drive letter on Windows
+	varLog, _ := filepath.Abs("/var/log")
 	d = LimaYAML{
 		VMType: ptr.Of("vz"),
 		OS:     ptr.Of("unknown"),
@@ -385,7 +390,7 @@ func TestFillDefault(t *testing.T) {
 
 		Mounts: []Mount{
 			{
-				Location: "/var/log",
+				Location: varLog,
 				Writable: ptr.Of(false),
 			},
 		},
@@ -593,7 +598,7 @@ func TestFillDefault(t *testing.T) {
 
 		Mounts: []Mount{
 			{
-				Location: "/var/log",
+				Location: varLog,
 				Writable: ptr.Of(true),
 				SSHFS: SSHFS{
 					Cache:          ptr.Of(false),
diff --git a/pkg/limayaml/validate.go b/pkg/limayaml/validate.go
@@ -155,6 +155,7 @@ func Validate(y *LimaYAML, warn bool) error {
 			return fmt.Errorf("field `mounts[%d].location` must be an absolute path, got %q",
 				i, f.Location)
 		}
+		// f.Location has already been expanded in FillDefaults(), but that function cannot return errors.
 		loc, err := localpathutil.Expand(f.Location)
 		if err != nil {
 			return fmt.Errorf("field `mounts[%d].location` refers to an unexpandable path: %q: %w", i, f.Location, err)
@@ -175,6 +176,10 @@ func Validate(y *LimaYAML, warn bool) error {
 		case *y.User.Home:
 			return fmt.Errorf("field `mounts[%d].mountPoint` is the reserved internal home directory %q", i, *y.User.Home)
 		}
+		// There is no tilde-expansion for guest filenames
+		if strings.HasPrefix(*f.MountPoint, "~") {
+			return fmt.Errorf("field `mounts[%d].mountPoint` must not start with \"~\"", i)
+		}
 
 		if _, err := units.RAMInBytes(*f.NineP.Msize); err != nil {
 			return fmt.Errorf("field `msize` has an invalid value: %w", err)
diff --git a/pkg/qemu/qemu.go b/pkg/qemu/qemu.go
@@ -31,7 +31,6 @@ import (
 	"github.com/lima-vm/lima/pkg/fileutils"
 	"github.com/lima-vm/lima/pkg/iso9660util"
 	"github.com/lima-vm/lima/pkg/limayaml"
-	"github.com/lima-vm/lima/pkg/localpathutil"
 	"github.com/lima-vm/lima/pkg/networks"
 	"github.com/lima-vm/lima/pkg/qemu/imgutil"
 	"github.com/lima-vm/lima/pkg/store"
@@ -935,19 +934,15 @@ func Cmdline(ctx context.Context, cfg Config) (exe string, args []string, err er
 	if *y.MountType == limayaml.NINEP || *y.MountType == limayaml.VIRTIOFS {
 		for i, f := range y.Mounts {
 			tag := fmt.Sprintf("mount%d", i)
-			location, err := localpathutil.Expand(f.Location)
-			if err != nil {
-				return "", nil, err
-			}
-			if err := os.MkdirAll(location, 0o755); err != nil {
+			if err := os.MkdirAll(f.Location, 0o755); err != nil {
 				return "", nil, err
 			}
 
 			switch *y.MountType {
 			case limayaml.NINEP:
 				options := "local"
 				options += fmt.Sprintf(",mount_tag=%s", tag)
-				options += fmt.Sprintf(",path=%s", location)
+				options += fmt.Sprintf(",path=%s", f.Location)
 				options += fmt.Sprintf(",security_model=%s", *f.NineP.SecurityModel)
 				if !*f.Writable {
 					options += ",readonly"
@@ -1067,10 +1062,6 @@ func FindVirtiofsd(qemuExe string) (string, error) {
 
 func VirtiofsdCmdline(cfg Config, mountIndex int) ([]string, error) {
 	mount := cfg.LimaYAML.Mounts[mountIndex]
-	location, err := localpathutil.Expand(mount.Location)
-	if err != nil {
-		return nil, err
-	}
 
 	vhostSock := filepath.Join(cfg.InstanceDir, fmt.Sprintf(filenames.VhostSock, mountIndex))
 	// qemu_driver has to wait for the socket to appear, so make sure any old ones are removed here.
@@ -1080,7 +1071,7 @@ func VirtiofsdCmdline(cfg Config, mountIndex int) ([]string, error) {
 
 	return []string{
 		"--socket-path", vhostSock,
-		"--shared-dir", location,
+		"--shared-dir", mount.Location,
 	}, nil
 }
 
diff --git a/pkg/vz/vm_darwin.go b/pkg/vz/vm_darwin.go
@@ -25,7 +25,6 @@ import (
 	"github.com/lima-vm/lima/pkg/driver"
 	"github.com/lima-vm/lima/pkg/iso9660util"
 	"github.com/lima-vm/lima/pkg/limayaml"
-	"github.com/lima-vm/lima/pkg/localpathutil"
 	"github.com/lima-vm/lima/pkg/nativeimgutil"
 	"github.com/lima-vm/lima/pkg/networks"
 	"github.com/lima-vm/lima/pkg/networks/usernet"
@@ -548,18 +547,14 @@ func attachFolderMounts(driver *driver.BaseDriver, vmConfig *vz.VirtualMachineCo
 	var mounts []vz.DirectorySharingDeviceConfiguration
 	if *driver.Instance.Config.MountType == limayaml.VIRTIOFS {
 		for i, mount := range driver.Instance.Config.Mounts {
-			expandedPath, err := localpathutil.Expand(mount.Location)
-			if err != nil {
-				return err
-			}
-			if _, err := os.Stat(expandedPath); errors.Is(err, os.ErrNotExist) {
-				err := os.MkdirAll(expandedPath, 0o750)
+			if _, err := os.Stat(mount.Location); errors.Is(err, os.ErrNotExist) {
+				err := os.MkdirAll(mount.Location, 0o750)
 				if err != nil {
 					return err
 				}
 			}
 
-			directory, err := vz.NewSharedDirectory(expandedPath, !*mount.Writable)
+			directory, err := vz.NewSharedDirectory(mount.Location, !*mount.Writable)
 			if err != nil {
 				return err
 			}

Original file line number	Diff line number	Diff line change
`@@ -29,6 +29,7 @@ import (`
`29`	`29`	`"golang.org/x/sys/cpu"`
`30`	`30`
`31`	`31`	`"github.com/lima-vm/lima/pkg/identifierutil"`
	`32`	`+ "github.com/lima-vm/lima/pkg/localpathutil"`
`32`	`33`	`. "github.com/lima-vm/lima/pkg/must"`
`33`	`34`	`"github.com/lima-vm/lima/pkg/networks"`
`34`	`35`	`"github.com/lima-vm/lima/pkg/osutil"`
`@@ -822,8 +823,13 @@ func FillDefault(y, d, o *LimaYAML, filePath string, warn bool) {`
`822`	`823`	`mounts[i].NineP.Cache = ptr.Of(Default9pCacheForRO)`
`823`	`824`	`}`
`824`	`825`	`}`
	`826`	`+ if location, err := localpathutil.Expand(mount.Location); err == nil {`
	`827`	`+ mounts[i].Location = location`
	`828`	`+ } else {`
	`829`	`+ logrus.WithError(err).Warnf("Couldn't expand location %q", mount.Location)`
	`830`	`+ }`
`825`	`831`	`if mount.MountPoint == nil {`
`826`		`- mounts[i].MountPoint = ptr.Of(mount.Location)`
	`832`	`+ mounts[i].MountPoint = ptr.Of(mounts[i].Location)`
`827`	`833`	`}`
`828`	`834`	`}`
`829`	`835`