Simplifies filesystem implementations (#1548)

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

internal/sysfs/dir.go

@@ -13,12 +13,12 @@ func adjustReaddirErr(f fsapi.File, isClosed bool, err error) syscall.Errno {
 		return 0 // e.g. Readdir on darwin returns io.EOF, but linux doesn't.
 	} else if errno := platform.UnwrapOSError(err); errno != 0 {
 		errno = dirError(f, isClosed, errno)
-		// Ignore errors when the file was closed or removed.
+		// Comply with errors allowed on fsapi.File Readdir
 		switch errno {
-		case syscall.EIO, syscall.EBADF: // closed while open
-			return 0
-		case syscall.ENOENT: // Linux error when removed while open
-			return 0
+		case syscall.EINVAL: // os.File Readdir can return this
+			return syscall.EBADF
+		case syscall.ENOTDIR: // dirError can return this
+			return syscall.EBADF
 		}
 		return errno
 	}

internal/sysfs/dir_test.go

@@ -4,7 +4,6 @@ import (
 	"io"
 	"io/fs"
 	"os"
-	"path"
 	"runtime"
 	"sort"
 	"syscall"
@@ -57,11 +56,12 @@ func TestReaddir(t *testing.T) {
 				testReaddirAll(t, dotF, tc.expectIno)
 			})
 
-			// Don't err if something else closed the directory while reading.
+			// Err if the caller closed the directory while reading. This is
+			// different from something else deleting it.
 			t.Run("closed dir", func(t *testing.T) {
 				require.EqualErrno(t, 0, dotF.Close())
 				_, errno := dotF.Readdir(-1)
-				require.EqualErrno(t, 0, errno)
+				require.EqualErrno(t, syscall.EBADF, errno)
 			})
 
 			fileF, errno := sysfs.OpenFSFile(tc.fs, "empty.txt", syscall.O_RDONLY, 0)
@@ -70,7 +70,7 @@ func TestReaddir(t *testing.T) {
 
 			t.Run("file", func(t *testing.T) {
 				_, errno := fileF.Readdir(-1)
-				require.EqualErrno(t, syscall.ENOTDIR, errno)
+				require.EqualErrno(t, syscall.EBADF, errno)
 			})
 
 			dirF, errno := sysfs.OpenFSFile(tc.fs, "dir", syscall.O_RDONLY, 0)
@@ -127,30 +127,6 @@ func TestReaddir(t *testing.T) {
 			})
 		})
 	}
-
-	// Don't err if something else removed the directory while reading.
-	t.Run("removed while open", func(t *testing.T) {
-		dirF, errno := sysfs.OpenFSFile(dirFS, "dir", syscall.O_RDONLY, 0)
-		require.EqualErrno(t, 0, errno)
-		defer dirF.Close()
-
-		dirents, errno := dirF.Readdir(1)
-		require.EqualErrno(t, 0, errno)
-		require.Equal(t, 1, len(dirents))
-
-		// Speculatively try to remove even if it won't likely work
-		// on windows.
-		err := os.RemoveAll(path.Join(tmpDir, "dir"))
-		if err != nil && runtime.GOOS == "windows" {
-			t.Skip()
-		} else {
-			require.NoError(t, err)
-		}
-
-		_, errno = dirF.Readdir(1)
-		require.EqualErrno(t, 0, errno)
-		// don't validate the contents as due to caching it might be present.
-	})
 }
 
 func testReaddirAll(t *testing.T, dotF fsapi.File, expectIno bool) {

internal/sysfs/dirfs_test.go

@@ -36,7 +36,7 @@ func TestNewDirFS(t *testing.T) {
 		d, errno := testFS.OpenFile(".", os.O_RDONLY, 0)
 		require.EqualErrno(t, 0, errno)
 		_, errno = d.Readdir(-1)
-		require.EqualErrno(t, syscall.ENOTDIR, errno)
+		require.EqualErrno(t, syscall.EBADF, errno)
 	})
 }
 
@@ -802,10 +802,7 @@ func TestDirFS_Truncate(t *testing.T) {
 	})
 }
 
-// Test_fdReaddir_opened_file_written ensures that writing files to the already-opened directory
-// is visible. This is significant on Windows.
-// https://github.com/ziglang/zig/blob/2ccff5115454bab4898bae3de88f5619310bc5c1/lib/std/fs/test.zig#L156-L184
-func Test_fdReaddir_opened_file_written(t *testing.T) {
+func TestDirFS_Readdir(t *testing.T) {
 	root := t.TempDir()
 	testFS := NewDirFS(root)
 
@@ -813,21 +810,46 @@ func Test_fdReaddir_opened_file_written(t *testing.T) {
 	errno := testFS.Mkdir(readDirTarget, 0o700)
 	require.EqualErrno(t, 0, errno)
 
-	// Open the directory, before writing files!
+	// Open the empty directory
 	dirFile, errno := testFS.OpenFile(readDirTarget, os.O_RDONLY, 0)
 	require.EqualErrno(t, 0, errno)
 	defer dirFile.Close()
 
-	// Then write a file to the directory.
-	f, err := os.Create(path.Join(root, readDirTarget, "my-file"))
-	require.NoError(t, err)
-	defer f.Close()
+	// Write files to the directory after it is open.
+	require.NoError(t, os.WriteFile(path.Join(root, readDirTarget, "1"), nil, 0o444))
+	require.NoError(t, os.WriteFile(path.Join(root, readDirTarget, "2"), nil, 0o444))
 
-	dirents, errno := dirFile.Readdir(-1)
-	require.EqualErrno(t, 0, errno)
+	// Test files are visible. This fails in windows unless the implementation
+	// re-opens the underlying file.
+	// https://github.com/ziglang/zig/blob/e3736baddb8ecff90f0594be9f604c7484ce9aa2/lib/std/fs/test.zig#L290-L317
+	t.Run("Sees files written after open", func(t *testing.T) {
+		dirents, errno := dirFile.Readdir(1)
+		require.EqualErrno(t, 0, errno)
 
-	require.Equal(t, 1, len(dirents))
-	require.Equal(t, "my-file", dirents[0].Name)
+		require.Equal(t, 1, len(dirents))
+		n := dirents[0].Name
+		switch n {
+		case "1", "2": // order is inconsistent on scratch images.
+		default:
+			require.Equal(t, "1", n)
+		}
+	})
+
+	// Test there is no error reading the directory if it was deleted while
+	// iterating. See docs on Readdir for why in general, but specifically Zig
+	// tests enforce this. This test is Windows sensitive as well.
+	//
+	// https://github.com/ziglang/zig/blob/e3736baddb8ecff90f0594be9f604c7484ce9aa2/lib/std/fs/test.zig#L311C1-L311C1
+	t.Run("syscall.ENOENT or no error, deleted while reading", func(t *testing.T) {
+		require.NoError(t, os.RemoveAll(path.Join(root, readDirTarget)))
+
+		dirents, errno := dirFile.Readdir(-1)
+		if errno != 0 {
+			require.EqualErrno(t, syscall.ENOENT, errno)
+		}
+
+		require.Equal(t, 0, len(dirents))
+	})
 }
 
 func TestDirFS_Link(t *testing.T) {

internal/sysfs/file.go

@@ -137,7 +137,7 @@ func (f *fsFile) cachedStat() (fileType fs.FileMode, ino uint64, errno syscall.E
 	return f.cachedSt.fileType, f.cachedSt.ino, 0
 }
 
-// Ino implements File.Ino
+// Ino implements the same method as documented on fsapi.File
 func (f *fsFile) Ino() (uint64, syscall.Errno) {
 	if _, ino, errno := f.cachedStat(); errno != 0 {
 		return 0, errno
@@ -146,17 +146,17 @@ func (f *fsFile) Ino() (uint64, syscall.Errno) {
 	}
 }
 
-// IsAppend implements File.IsAppend
+// IsAppend implements the same method as documented on fsapi.File
 func (f *fsFile) IsAppend() bool {
 	return false
 }
 
-// SetAppend implements File.SetAppend
+// SetAppend implements the same method as documented on fsapi.File
 func (f *fsFile) SetAppend(bool) (errno syscall.Errno) {
 	return fileError(f, f.closed, syscall.ENOSYS)
 }
 
-// IsDir implements File.IsDir
+// IsDir implements the same method as documented on fsapi.File
 func (f *fsFile) IsDir() (bool, syscall.Errno) {
 	if ft, _, errno := f.cachedStat(); errno != 0 {
 		return false, errno
@@ -166,7 +166,7 @@ func (f *fsFile) IsDir() (bool, syscall.Errno) {
 	return false, 0
 }
 
-// Stat implements File.Stat
+// Stat implements the same method as documented on fsapi.File
 func (f *fsFile) Stat() (st fsapi.Stat_t, errno syscall.Errno) {
 	if f.closed {
 		errno = syscall.EBADF
@@ -195,7 +195,7 @@ func (f *fsFile) cacheStat(st fsapi.Stat_t) (fsapi.Stat_t, syscall.Errno) {
 	return st, 0
 }
 
-// Read implements File.Read
+// Read implements the same method as documented on fsapi.File
 func (f *fsFile) Read(buf []byte) (n int, errno syscall.Errno) {
 	if n, errno = read(f.file, buf); errno != 0 {
 		// Defer validation overhead until we've already had an error.
@@ -204,7 +204,7 @@ func (f *fsFile) Read(buf []byte) (n int, errno syscall.Errno) {
 	return
 }
 
-// Pread implements File.Pread
+// Pread implements the same method as documented on fsapi.File
 func (f *fsFile) Pread(buf []byte, off int64) (n int, errno syscall.Errno) {
 	if ra, ok := f.file.(io.ReaderAt); ok {
 		if n, errno = pread(ra, buf, off); errno != 0 {
@@ -243,7 +243,7 @@ func (f *fsFile) Pread(buf []byte, off int64) (n int, errno syscall.Errno) {
 	return
 }
 
-// Seek implements File.Seek.
+// Seek implements the same method as documented on fsapi.File
 func (f *fsFile) Seek(offset int64, whence int) (newOffset int64, errno syscall.Errno) {
 	// If this is a directory, and we're attempting to seek to position zero,
 	// we have to re-open the file to ensure the directory state is reset.
@@ -267,16 +267,17 @@ func (f *fsFile) Seek(offset int64, whence int) (newOffset int64, errno syscall.
 	return
 }
 
-func (f *fsFile) reopen() syscall.Errno {
-	_ = f.close()
-	var err error
-	f.file, err = f.fs.Open(f.name)
-	return platform.UnwrapOSError(err)
-}
-
 // Readdir implements File.Readdir. Notably, this uses fs.ReadDirFile if
 // available.
 func (f *fsFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) {
+	// Windows lets you Readdir after close, fs.File also may not implement
+	// close in a meaningful way. read our closed field to return consistent
+	// results.
+	if f.closed {
+		errno = syscall.EBADF
+		return
+	}
+
 	if of, ok := f.file.(*os.File); ok {
 		// We can't use f.name here because it is the path up to the fsapi.FS,
 		// not necessarily the real path. For this reason, Windows may not be
@@ -300,12 +301,12 @@ func (f *fsFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) {
 			dirents = append(dirents, fsapi.Dirent{Name: e.Name(), Type: e.Type()})
 		}
 	} else {
-		errno = syscall.ENOTDIR
+		errno = syscall.EBADF // not a directory
 	}
 	return
 }
 
-// Write implements File.Write
+// Write implements the same method as documented on fsapi.File.
 func (f *fsFile) Write(buf []byte) (n int, errno syscall.Errno) {
 	if w, ok := f.file.(io.Writer); ok {
 		if n, errno = write(w, buf); errno != 0 {
@@ -318,7 +319,7 @@ func (f *fsFile) Write(buf []byte) (n int, errno syscall.Errno) {
 	return
 }
 
-// Pwrite implements File.Pwrite
+// Pwrite implements the same method as documented on fsapi.File.
 func (f *fsFile) Pwrite(buf []byte, off int64) (n int, errno syscall.Errno) {
 	if wa, ok := f.file.(io.WriterAt); ok {
 		if n, errno = pwrite(wa, buf, off); errno != 0 {
@@ -331,7 +332,7 @@ func (f *fsFile) Pwrite(buf []byte, off int64) (n int, errno syscall.Errno) {
 	return
 }
 
-// Close implements File.Close
+// Close implements the same method as documented on fsapi.File.
 func (f *fsFile) Close() syscall.Errno {
 	if f.closed {
 		return 0
@@ -406,6 +407,21 @@ func seek(s io.Seeker, offset int64, whence int) (int64, syscall.Errno) {
 	return newOffset, platform.UnwrapOSError(err)
 }
 
+// reopenFile allows re-opening a file for reasons such as applying flags or
+// directory iteration.
+type reopenFile func() syscall.Errno
+
+// compile-time check to ensure fsFile.reopen implements reopenFile.
+var _ reopenFile = (*fsFile)(nil).reopen
+
+// reopen implements the same method as documented on reopenFile.
+func (f *fsFile) reopen() syscall.Errno {
+	_ = f.close()
+	var err error
+	f.file, err = f.fs.Open(f.name)
+	return platform.UnwrapOSError(err)
+}
+
 func readdir(f *os.File, path string, n int) (dirents []fsapi.Dirent, errno syscall.Errno) {
 	fis, e := f.Readdir(n)
 	if errno = platform.UnwrapOSError(e); errno != 0 {

internal/sysfs/open_file.go

@@ -7,14 +7,9 @@ import (
 	"os"
 	"syscall"
 
-	"github.com/tetratelabs/wazero/internal/fsapi"
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File {
-	return newDefaultOsFile(openPath, openFlag, openPerm, f)
-}
-
 // OpenFile is like os.OpenFile except it returns syscall.Errno. A zero
 // syscall.Errno is success.
 func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) {

internal/sysfs/open_file_js.go

@@ -8,10 +8,6 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) File {
-	return newDefaultOsFile(openPath, openFlag, openPerm, f)
-}
-
 func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) {
 	flag &= ^(O_DIRECTORY | O_NOFOLLOW) // erase placeholders
 	f, err := os.OpenFile(path, flag, perm)

internal/sysfs/open_file_sun.go

@@ -7,14 +7,9 @@ import (
 	"os"
 	"syscall"
 
-	"github.com/tetratelabs/wazero/internal/fsapi"
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File {
-	return newDefaultOsFile(openPath, openFlag, openPerm, f)
-}
-
 func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) {
 	f, err := os.OpenFile(path, flag, perm)
 	return f, platform.UnwrapOSError(err)

internal/sysfs/open_file_windows.go

@@ -11,12 +11,6 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File {
-	return &windowsOsFile{
-		osFile: osFile{path: openPath, flag: openFlag, perm: openPerm, file: f, fd: f.Fd()},
-	}
-}
-
 func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) {
 	isDir := flag&fsapi.O_DIRECTORY > 0
 	flag &= ^(fsapi.O_DIRECTORY | fsapi.O_NOFOLLOW) // erase placeholders
@@ -154,46 +148,3 @@ func open(path string, mode int, perm uint32) (fd syscall.Handle, err error) {
 	h, e := syscall.CreateFile(pathp, access, sharemode, sa, createmode, attrs, 0)
 	return h, e
 }
-
-// windowsOsFile overrides osFile to special case directory handling in Windows.
-type windowsOsFile struct {
-	osFile
-
-	dirInitialized bool
-}
-
-// Readdir implements File.Readdir
-func (f *windowsOsFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) {
-	if errno = f.maybeInitDir(); errno != 0 {
-		return
-	}
-
-	return f.osFile.Readdir(n)
-}
-
-func (f *windowsOsFile) maybeInitDir() syscall.Errno {
-	if f.dirInitialized {
-		return 0
-	}
-
-	if isDir, errno := f.IsDir(); errno != 0 {
-		return errno
-	} else if !isDir {
-		return syscall.ENOTDIR
-	}
-
-	// On Windows, once the directory is opened, changes to the directory are
-	// not visible on ReadDir on that already-opened file handle.
-	//
-	// To provide consistent behavior with other platforms, we re-open it.
-	if errno := f.osFile.Close(); errno != 0 {
-		return errno
-	}
-	newW, errno := openFile(f.path, f.flag, f.perm)
-	if errno != 0 {
-		return errno
-	}
-	f.osFile.file = newW
-	f.dirInitialized = true
-	return 0
-}

internal/sysfs/osfile.go

@@ -4,6 +4,7 @@ import (
 	"io"
 	"io/fs"
 	"os"
+	"runtime"
 	"syscall"
 	"time"
 
@@ -11,8 +12,12 @@ import (
 	"github.com/tetratelabs/wazero/internal/platform"
 )
 
-func newDefaultOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File {
-	return &osFile{path: openPath, flag: openFlag, perm: openPerm, file: f, fd: f.Fd()}
+func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File {
+	// Windows cannot read files written to a directory after it was opened.
+	// This was noticed in #1087 in zig tests. Use a flag instead of a
+	// different type.
+	reopenDir := runtime.GOOS == "windows"
+	return &osFile{path: openPath, flag: openFlag, perm: openPerm, reopenDir: reopenDir, file: f, fd: f.Fd()}
 }
 
 // osFile is a file opened with this package, and uses os.File or syscalls to
@@ -24,6 +29,9 @@ type osFile struct {
 	file *os.File
 	fd   uintptr
 
+	// reopenDir is true if reopen should be called before Readdir.
+	reopenDir bool
+
 	// closed is true when closed was called. This ensures proper syscall.EBADF
 	closed bool
 
@@ -72,6 +80,9 @@ func (f *osFile) SetAppend(enable bool) (errno syscall.Errno) {
 	return fileError(f, f.closed, f.reopen())
 }
 
+// compile-time check to ensure osFile.reopen implements reopenFile.
+var _ reopenFile = (*fsFile)(nil).reopen
+
 func (f *osFile) reopen() (errno syscall.Errno) {
 	// Clear any create flag, as we are re-opening, not re-creating.
 	f.flag &= ^syscall.O_CREAT
@@ -183,6 +194,13 @@ func (f *osFile) PollRead(timeout *time.Duration) (ready bool, errno syscall.Err
 // Readdir implements File.Readdir. Notably, this uses "Readdir", not
 // "ReadDir", from os.File.
 func (f *osFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) {
+	if f.reopenDir { // Lazy re-open the directory if needed.
+		f.reopenDir = false
+		if errno = adjustReaddirErr(f, f.closed, f.reopen()); errno != 0 {
+			return
+		}
+	}
+
 	if dirents, errno = readdir(f.file, f.path, n); errno != 0 {
 		errno = adjustReaddirErr(f, f.closed, errno)
 	}