sysfs: cleanup windows rename (#1584)

Signed-off-by: Adrian Cole <adrian@tetrate.io>

internal/sysfs/dirfs.go

@@ -70,7 +70,7 @@ func (d *dirFS) Chmod(path string, perm fs.FileMode) syscall.Errno {
 // Rename implements the same method as documented on fsapi.FS
 func (d *dirFS) Rename(from, to string) syscall.Errno {
 	from, to = d.join(from), d.join(to)
-	return Rename(from, to)
+	return rename(from, to)
 }
 
 // Readlink implements the same method as documented on fsapi.FS
@@ -92,13 +92,17 @@ func (d *dirFS) Link(oldName, newName string) syscall.Errno {
 
 // Rmdir implements the same method as documented on fsapi.FS
 func (d *dirFS) Rmdir(path string) syscall.Errno {
-	err := syscall.Rmdir(d.join(path))
+	return rmdir(d.join(path))
+}
+
+func rmdir(path string) syscall.Errno {
+	err := syscall.Rmdir(path)
 	return platform.UnwrapOSError(err)
 }
 
 // Unlink implements the same method as documented on fsapi.FS
 func (d *dirFS) Unlink(path string) (err syscall.Errno) {
-	return Unlink(d.join(path))
+	return unlink(d.join(path))
 }
 
 // Symlink implements the same method as documented on fsapi.FS

internal/sysfs/file_unix.go

@@ -8,7 +8,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-const NonBlockingFileIoSupported = true
+const nonBlockingFileIoSupported = true
 
 // readFd exposes syscall.Read.
 func readFd(fd uintptr, buf []byte) (int, syscall.Errno) {

internal/sysfs/file_unsupported.go

@@ -4,7 +4,7 @@ package sysfs
 
 import "syscall"
 
-const NonBlockingFileIoSupported = false
+const nonBlockingFileIoSupported = false
 
 // readFd returns ENOSYS on unsupported platforms.
 func readFd(fd uintptr, buf []byte) (int, syscall.Errno) {

internal/sysfs/file_windows.go

@@ -7,7 +7,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-const NonBlockingFileIoSupported = true
+const nonBlockingFileIoSupported = true
 
 var kernel32 = syscall.NewLazyDLL("kernel32.dll")
 

internal/sysfs/osfile.go

@@ -144,7 +144,7 @@ func (f *osFile) Read(buf []byte) (n int, errno syscall.Errno) {
 	if len(buf) == 0 {
 		return 0, 0 // Short-circuit 0-len reads.
 	}
-	if NonBlockingFileIoSupported && f.IsNonblock() {
+	if nonBlockingFileIoSupported && f.IsNonblock() {
 		n, errno = readFd(f.fd, buf)
 	} else {
 		n, errno = read(f.file, buf)

internal/sysfs/rename.go

@@ -8,7 +8,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func Rename(from, to string) syscall.Errno {
+func rename(from, to string) syscall.Errno {
 	if from == to {
 		return 0
 	}

internal/sysfs/rename_test.go

@@ -19,7 +19,7 @@ func TestRename(t *testing.T) {
 		err := os.WriteFile(file1Path, []byte{1}, 0o600)
 		require.NoError(t, err)
 
-		err = Rename(path.Join(tmpDir, "non-exist"), file1Path)
+		err = rename(path.Join(tmpDir, "non-exist"), file1Path)
 		require.EqualErrno(t, syscall.ENOENT, err)
 	})
 	t.Run("file to non-exist", func(t *testing.T) {
@@ -31,7 +31,7 @@ func TestRename(t *testing.T) {
 		require.NoError(t, err)
 
 		file2Path := path.Join(tmpDir, "file2")
-		errno := Rename(file1Path, file2Path)
+		errno := rename(file1Path, file2Path)
 		require.EqualErrno(t, 0, errno)
 
 		// Show the prior path no longer exists
@@ -49,7 +49,7 @@ func TestRename(t *testing.T) {
 		require.NoError(t, os.Mkdir(dir1Path, 0o700))
 
 		dir2Path := path.Join(tmpDir, "dir2")
-		errno := Rename(dir1Path, dir2Path)
+		errno := rename(dir1Path, dir2Path)
 		require.EqualErrno(t, 0, errno)
 
 		// Show the prior path no longer exists
@@ -72,7 +72,7 @@ func TestRename(t *testing.T) {
 		err := os.WriteFile(dir2Path, []byte{2}, 0o600)
 		require.NoError(t, err)
 
-		err = Rename(dir1Path, dir2Path)
+		err = rename(dir1Path, dir2Path)
 		require.EqualErrno(t, syscall.ENOTDIR, err)
 	})
 	t.Run("file to dir", func(t *testing.T) {
@@ -86,7 +86,7 @@ func TestRename(t *testing.T) {
 		dir1Path := path.Join(tmpDir, "dir1")
 		require.NoError(t, os.Mkdir(dir1Path, 0o700))
 
-		err = Rename(file1Path, dir1Path)
+		err = rename(file1Path, dir1Path)
 		require.EqualErrno(t, syscall.EISDIR, err)
 	})
 
@@ -108,7 +108,7 @@ func TestRename(t *testing.T) {
 		dir2Path := path.Join(tmpDir, "dir2")
 		require.NoError(t, os.Mkdir(dir2Path, 0o700))
 
-		errno := Rename(dir1Path, dir2Path)
+		errno := rename(dir1Path, dir2Path)
 		require.EqualErrno(t, 0, errno)
 
 		// Show the prior path no longer exists
@@ -142,7 +142,7 @@ func TestRename(t *testing.T) {
 		err = os.WriteFile(path.Join(dir2Path, "existing.txt"), []byte("any thing"), 0o600)
 		require.NoError(t, err)
 
-		err = Rename(dir1Path, dir2Path)
+		err = rename(dir1Path, dir2Path)
 		require.EqualErrno(t, syscall.ENOTEMPTY, err)
 	})
 
@@ -159,7 +159,7 @@ func TestRename(t *testing.T) {
 		err = os.WriteFile(file2Path, file2Contents, 0o600)
 		require.NoError(t, err)
 
-		errno := Rename(file1Path, file2Path)
+		errno := rename(file1Path, file2Path)
 		require.EqualErrno(t, 0, errno)
 
 		// Show the prior path no longer exists
@@ -177,7 +177,7 @@ func TestRename(t *testing.T) {
 		dir1Path := path.Join(tmpDir, "dir1")
 		require.NoError(t, os.Mkdir(dir1Path, 0o700))
 
-		errno := Rename(dir1Path, dir1Path)
+		errno := rename(dir1Path, dir1Path)
 		require.EqualErrno(t, 0, errno)
 
 		s, err := os.Stat(dir1Path)
@@ -192,7 +192,7 @@ func TestRename(t *testing.T) {
 		err := os.WriteFile(file1Path, file1Contents, 0o600)
 		require.NoError(t, err)
 
-		errno := Rename(file1Path, file1Path)
+		errno := rename(file1Path, file1Path)
 		require.EqualErrno(t, 0, errno)
 
 		b, err := os.ReadFile(file1Path)

internal/sysfs/rename_windows.go

@@ -1,47 +1,55 @@
 package sysfs
 
 import (
-	"errors"
 	"os"
 	"syscall"
 
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func Rename(from, to string) syscall.Errno {
+func rename(from, to string) syscall.Errno {
 	if from == to {
 		return 0
 	}
 
-	fromStat, err := os.Stat(from)
-	if err != nil {
-		return syscall.ENOENT
+	var fromIsDir, toIsDir bool
+	if fromStat, errno := stat(from); errno != 0 {
+		return errno // failed to stat from
+	} else {
+		fromIsDir = fromStat.Mode.IsDir()
+	}
+	if toStat, errno := stat(to); errno == syscall.ENOENT {
+		return syscallRename(from, to) // file or dir to not-exist is ok
+	} else if errno != 0 {
+		return errno // failed to stat to
+	} else {
+		toIsDir = toStat.Mode.IsDir()
 	}
 
-	if toStat, err := os.Stat(to); err == nil {
-		fromIsDir, toIsDir := fromStat.IsDir(), toStat.IsDir()
-		if fromIsDir && !toIsDir { // dir to file
-			return syscall.ENOTDIR
-		} else if !fromIsDir && toIsDir { // file to dir
-			return syscall.EISDIR
-		} else if !fromIsDir && !toIsDir { // file to file
-			// Use os.Rename instead of syscall.Rename in order to allow the overrides of the existing file.
-			// Underneath os.Rename, it uses MoveFileEx instead of MoveFile (used by syscall.Rename).
-			return platform.UnwrapOSError(os.Rename(from, to))
-		} else { // dir to dir
-			if dirs, _ := os.ReadDir(to); len(dirs) == 0 {
-				// On Windows, renaming to the empty dir will be rejected,
-				// so first we remove the empty dir, and then rename to it.
-				if err := os.Remove(to); err != nil {
-					return platform.UnwrapOSError(err)
-				}
-				return platform.UnwrapOSError(syscall.Rename(from, to))
-			}
-			return syscall.ENOTEMPTY
+	// Now, handle known cases
+	switch {
+	case !fromIsDir && toIsDir: // file to dir
+		return syscall.EISDIR
+	case !fromIsDir && !toIsDir: // file to file
+		// Use os.Rename instead of syscall.Rename to overwrite a file.
+		// This uses MoveFileEx instead of MoveFile (used by syscall.Rename).
+		return platform.UnwrapOSError(os.Rename(from, to))
+	case fromIsDir && !toIsDir: // dir to file
+		return syscall.ENOTDIR
+	default: // dir to dir
+
+		// We can't tell if a directory is empty or not, via stat information.
+		// Reading the directory is expensive, as it can buffer large amounts
+		// of data on fail. Instead, speculatively try to remove the directory.
+		// This is only one syscall and won't buffer anything.
+		if errno := rmdir(to); errno == 0 || errno == syscall.ENOENT {
+			return syscallRename(from, to)
+		} else {
+			return errno
 		}
-	} else if !errors.Is(err, syscall.ENOENT) { // Failed to stat the destination.
-		return platform.UnwrapOSError(err)
-	} else { // Destination not-exist.
-		return platform.UnwrapOSError(syscall.Rename(from, to))
 	}
 }
+
+func syscallRename(from string, to string) syscall.Errno {
+	return platform.UnwrapOSError(syscall.Rename(from, to))
+}

internal/sysfs/stat_bsd.go

@@ -11,6 +11,12 @@ import (
 	"github.com/tetratelabs/wazero/sys"
 )
 
+// dirNlinkIncludesDot is true because even though os.File filters out dot
+// entries, the underlying syscall.Stat includes them.
+//
+// Note: this is only used in tests
+const dirNlinkIncludesDot = true
+
 func lstat(path string) (sys.Stat_t, syscall.Errno) {
 	if info, err := os.Lstat(path); err != nil {
 		return sys.Stat_t{}, platform.UnwrapOSError(err)

internal/sysfs/stat_linux.go

@@ -14,6 +14,12 @@ import (
 	"github.com/tetratelabs/wazero/sys"
 )
 
+// dirNlinkIncludesDot is true because even though os.File filters out dot
+// entries, the underlying syscall.Stat includes them.
+//
+// Note: this is only used in tests
+const dirNlinkIncludesDot = true
+
 func lstat(path string) (sys.Stat_t, syscall.Errno) {
 	if info, err := os.Lstat(path); err != nil {
 		return sys.Stat_t{}, platform.UnwrapOSError(err)

internal/sysfs/stat_test.go

@@ -23,14 +23,50 @@ func TestStat(t *testing.T) {
 
 	var st sys.Stat_t
 
-	t.Run("dir", func(t *testing.T) {
+	t.Run("empty dir", func(t *testing.T) {
 		st, errno = stat(tmpDir)
 		require.EqualErrno(t, 0, errno)
 
 		require.True(t, st.Mode.IsDir())
 		require.NotEqual(t, uint64(0), st.Ino)
+
+		// We expect one link: the directory itself
+		expectedNlink := uint64(1)
+		if dirNlinkIncludesDot {
+			expectedNlink++
+		}
+		require.Equal(t, expectedNlink, st.Nlink, runtime.GOOS)
 	})
 
+	subdir := path.Join(tmpDir, "sub")
+	var stSubdir sys.Stat_t
+	t.Run("subdir", func(t *testing.T) {
+		require.NoError(t, os.Mkdir(subdir, 0o500))
+
+		stSubdir, errno = stat(subdir)
+		require.EqualErrno(t, 0, errno)
+
+		require.True(t, stSubdir.Mode.IsDir())
+		require.NotEqual(t, uint64(0), st.Ino)
+	})
+
+	t.Run("not empty dir", func(t *testing.T) {
+		st, errno = stat(tmpDir)
+		require.EqualErrno(t, 0, errno)
+
+		// We expect two links: the directory itself and the subdir
+		expectedNlink := uint64(2)
+		if dirNlinkIncludesDot {
+			expectedNlink++
+		} else if runtime.GOOS == "windows" {
+			expectedNlink = 1 // directory count is not returned.
+		}
+		require.Equal(t, expectedNlink, st.Nlink, runtime.GOOS)
+	})
+
+	// TODO: Investigate why Nlink increases on BSD when a file is added, but
+	// not Linux.
+
 	file := path.Join(tmpDir, "file")
 	var stFile sys.Stat_t
 
@@ -54,18 +90,6 @@ func TestStat(t *testing.T) {
 		require.Equal(t, stFile, stLink) // resolves to the file
 	})
 
-	subdir := path.Join(tmpDir, "sub")
-	var stSubdir sys.Stat_t
-	t.Run("subdir", func(t *testing.T) {
-		require.NoError(t, os.Mkdir(subdir, 0o500))
-
-		stSubdir, errno = stat(subdir)
-		require.EqualErrno(t, 0, errno)
-
-		require.True(t, stSubdir.Mode.IsDir())
-		require.NotEqual(t, uint64(0), st.Ino)
-	})
-
 	t.Run("link to dir", func(t *testing.T) {
 		link := path.Join(tmpDir, "dir-link")
 		require.NoError(t, os.Symlink(subdir, link))
@@ -249,7 +273,7 @@ func TestStatFile_dev_inode(t *testing.T) {
 	require.EqualErrno(t, 0, l2.Close())
 
 	// Renaming a file shouldn't change its inodes.
-	require.EqualErrno(t, 0, Rename(path1, path2))
+	require.EqualErrno(t, 0, rename(path1, path2))
 	f1 = requireOpenFile(t, path2, os.O_RDONLY, 0)
 	defer f1.Close()
 

internal/sysfs/stat_unsupported.go

@@ -14,6 +14,12 @@ import (
 // Note: go:build constraints must be the same as /sys.stat_unsupported.go for
 // the same reasons.
 
+// dirNlinkIncludesDot might be true for some operating systems, which can have
+// new stat_XX.go files as necessary.
+//
+// Note: this is only used in tests
+const dirNlinkIncludesDot = false
+
 func lstat(path string) (sys.Stat_t, syscall.Errno) {
 	if info, err := os.Lstat(path); err != nil {
 		return sys.Stat_t{}, platform.UnwrapOSError(err)

internal/sysfs/stat_windows.go

@@ -11,6 +11,11 @@ import (
 	"github.com/tetratelabs/wazero/sys"
 )
 
+// dirNlinkIncludesDot is false because Windows does not return dot entries.
+//
+// Note: this is only used in tests
+const dirNlinkIncludesDot = false
+
 func lstat(path string) (sys.Stat_t, syscall.Errno) {
 	attrs := uint32(syscall.FILE_FLAG_BACKUP_SEMANTICS)
 	// Use FILE_FLAG_OPEN_REPARSE_POINT, otherwise CreateFile will follow symlink.

internal/sysfs/unlink.go

@@ -8,7 +8,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func Unlink(name string) (errno syscall.Errno) {
+func unlink(name string) (errno syscall.Errno) {
 	err := syscall.Unlink(name)
 	if errno = platform.UnwrapOSError(err); errno == syscall.EPERM {
 		errno = syscall.EISDIR

internal/sysfs/unlink_test.go

@@ -12,7 +12,7 @@ import (
 func TestUnlink(t *testing.T) {
 	t.Run("doesn't exist", func(t *testing.T) {
 		name := "non-existent"
-		errno := Unlink(name)
+		errno := unlink(name)
 		require.EqualErrno(t, syscall.ENOENT, errno)
 	})
 
@@ -22,7 +22,7 @@ func TestUnlink(t *testing.T) {
 		dir := path.Join(tmpDir, "dir")
 		require.NoError(t, os.Mkdir(dir, 0o700))
 
-		errno := Unlink(dir)
+		errno := unlink(dir)
 		require.EqualErrno(t, syscall.EISDIR, errno)
 
 		require.NoError(t, os.Remove(dir))
@@ -40,7 +40,7 @@ func TestUnlink(t *testing.T) {
 		require.NoError(t, os.Symlink("subdir", symlinkName))
 
 		// Unlinking the symlink should suceed.
-		errno := Unlink(symlinkName)
+		errno := unlink(symlinkName)
 		require.EqualErrno(t, 0, errno)
 	})
 
@@ -51,7 +51,7 @@ func TestUnlink(t *testing.T) {
 
 		require.NoError(t, os.WriteFile(name, []byte{}, 0o600))
 
-		require.EqualErrno(t, 0, Unlink(name))
+		require.EqualErrno(t, 0, unlink(name))
 		_, err := os.Stat(name)
 		require.Error(t, err)
 	})

internal/sysfs/unlink_windows.go

@@ -9,7 +9,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func Unlink(name string) syscall.Errno {
+func unlink(name string) syscall.Errno {
 	err := syscall.Unlink(name)
 	if err == nil {
 		return 0