Repository: tetratelabs/wazero
Pull request #1584 (2023-07-15 02:25:14)

sysfs: cleanup windows rename
This makes windows rename implementation cheaper and also consolidates errno conversion. It is helpful to consolidate more before we expose our own Errno in #1582

Minor note, but this also unexports a few symbols that were only used in the same package.

codefromthecrypt: https://github.com/tetratelabs/wazero/pull/1582 now passes (dependent on this PR)

Patch:
diff --git a/internal/sysfs/dirfs.go b/internal/sysfs/dirfs.go
index e6a49ae2..acf1c3b8 100644
--- a/internal/sysfs/dirfs.go
+++ b/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
diff --git a/internal/sysfs/file_unix.go b/internal/sysfs/file_unix.go
index e451df82..d549a0e1 100644
--- a/internal/sysfs/file_unix.go
+++ b/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) {
diff --git a/internal/sysfs/file_unsupported.go b/internal/sysfs/file_unsupported.go
index 3b45d7bc..c10d39aa 100644
--- a/internal/sysfs/file_unsupported.go
+++ b/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) {
diff --git a/internal/sysfs/file_windows.go b/internal/sysfs/file_windows.go
index 448726fe..b72fe364 100644
--- a/internal/sysfs/file_windows.go
+++ b/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")
 
diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go
index 7c797526..a306edff 100644
--- a/internal/sysfs/osfile.go
+++ b/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)
diff --git a/internal/sysfs/rename.go b/internal/sysfs/rename.go
index b107bc19..d558a7a5 100644
--- a/internal/sysfs/rename.go
+++ b/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
 	}
diff --git a/internal/sysfs/rename_windows.go b/internal/sysfs/rename_windows.go
index 25e53d4f..1d38faad 100644
--- a/internal/sysfs/rename_windows.go
+++ b/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()
+	}
+
+	// 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
 
-	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
+		// 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))
+}
diff --git a/internal/sysfs/stat_bsd.go b/internal/sysfs/stat_bsd.go
index f24b6355..ed21f87c 100644
--- a/internal/sysfs/stat_bsd.go
+++ b/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)
diff --git a/internal/sysfs/stat_linux.go b/internal/sysfs/stat_linux.go
index d8085673..c48e5f74 100644
--- a/internal/sysfs/stat_linux.go
+++ b/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)
diff --git a/internal/sysfs/stat_unsupported.go b/internal/sysfs/stat_unsupported.go
index 5bb3c3da..12330070 100644
--- a/internal/sysfs/stat_unsupported.go
+++ b/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)
diff --git a/internal/sysfs/stat_windows.go b/internal/sysfs/stat_windows.go
index 575c984c..973b574e 100644
--- a/internal/sysfs/stat_windows.go
+++ b/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.
diff --git a/internal/sysfs/unlink.go b/internal/sysfs/unlink.go
index 37b74e18..37b47517 100644
--- a/internal/sysfs/unlink.go
+++ b/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
diff --git a/internal/sysfs/unlink_windows.go b/internal/sysfs/unlink_windows.go
index 410ea844..ca7cdeb3 100644
--- a/internal/sysfs/unlink_windows.go
+++ b/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
diff --git a/sys/stat.go b/sys/stat.go
index f2cc3845..bb7b9e5d 100644
--- a/sys/stat.go
+++ b/sys/stat.go
@@ -52,7 +52,11 @@ type Stat_t struct {
 	// type of the file (fs.ModeType) and its permissions (fs.ModePerm).
 	Mode fs.FileMode
 
-	/// Nlink is the number of hard links to the file.
+	// Nlink is the number of hard links to the file.
+	//
+	// Note: This value is platform-specific and often at least one. Linux will
+	// return 1+N for a directory, where BSD (like Darwin) return 2+N, which
+	// includes the dot entry.
 	Nlink uint64
 
 	// Size is the length in bytes for regular files. For symbolic links, this


Test patch:
diff --git a/internal/sysfs/rename_test.go b/internal/sysfs/rename_test.go
index 84d17f33..89ed94b9 100644
--- a/internal/sysfs/rename_test.go
+++ b/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)
diff --git a/internal/sysfs/stat_test.go b/internal/sysfs/stat_test.go
index 4c279ecb..2f714a2f 100644
--- a/internal/sysfs/stat_test.go
+++ b/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()
 
diff --git a/internal/sysfs/unlink_test.go b/internal/sysfs/unlink_test.go
index b44261e7..b7f837f6 100644
--- a/internal/sysfs/unlink_test.go
+++ b/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)
 	})