Repository: tetratelabs/wazero
Pull request #1026 (2023-01-12 13:58:22)

fs: Adds fd_sync
This adds the fd_sync syscall:
https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#-fd_syncfd-fd---errno

ncruces: I need this to get `test_demovfs.c` to work on [`github.com/ncruces/go-sqlite3`](https://github.com/ncruces/go-sqlite3).

The intent is to (eventually) replace this VFS by one that sidesteps WASI but, meanwhile, SQLite might be a nice stress test of WASI IO.

Unit tests here are far from ideal. Any ideas?

If you're curious about testing `go-sqlite3` you can do this:
```sh
git clone --branch file https://github.com/ncruces/go-sqlite3
cd go-sqlite3
./build_deps.sh
go run ./cmd
```

The `build_deps.sh` stage downloads and builds the SQLite amalgamation with `zig cc`.
`go run` creates a `test.db` which you can then inspect with the `sqlite3` console.
ncruces: Tests are failing on Windows, because of (not sure how to fix it):
https://github.com/tetratelabs/wazero/blob/main/internal/syscallfs/syscall_windows.go#L96
ncruces: Because of #1027, `go-sqlite3` doesn't really succeed if this gets merged.
I need to patch this on top of 859ca2451c12dc62145c52d5e4a7517570bf6cb2, just before bedde6dc7a3dd279f0e19ccd1a6d100570d996bd got merged.
codefromthecrypt: @ncruces thanks for progressing this. I have a windows laptop with me and so can help on this PR once I finish the pre-open thing I'm working on. Fantastic use case, btw!
codefromthecrypt: actually the pre-opens thing will take longer. will look at this now
codefromthecrypt: TL;DR; I think you can for now make the test skip on windows with a TODO, and I can handle it later.

e.g. `if runtime.GOOS == "windows { t.Skip("TODO: windows currently fails on EBADFD") }`

I can't immediately think of a semantic test we can do for linux/darwin etc, besides just making sure it doesn't error.

---

My laptop is stuck in windows update and it is really slow, so I'll mention what I think so far.

As far as I can tell, go doesn't test `File.Sync()` or Fsync for platforms that end up calling it. Hence, I'm not sure how much we can do about this. What's odd is os.File is defined in file_posix.go and the latter is defined for windows. So, I think the Sync method exists, and the EBADFD is for another reason maybe.

Not directly related, but seems we have another mapping thing to do, if we start testing O_SYNC (or attempting to) https://github.com/golang/go/issues/35358
ncruces: > As far as I can tell, go doesn't test `File.Sync()` or Fsync for platforms that end up calling it. Hence, I'm not sure how much we can do about this. What's odd is os.File is defined in file_posix.go and the latter is defined for windows. So, I think the Sync method exists, and the EBADFD is for another reason maybe.

Yes, on Windows `os.File.Sync()` exists (it exists on all platforms). The thing is, you're wrapping the file on that [anonymous struct](https://github.com/tetratelabs/wazero/blob/ecac3f529b8db1dae13a95933a0aa15b0ccdf530/internal/syscallfs/syscall_windows.go#L96) that only implements the [read](https://github.com/tetratelabs/wazero/blob/ecac3f529b8db1dae13a95933a0aa15b0ccdf530/internal/syscallfs/syscallfs.go#L134) and [write](https://github.com/tetratelabs/wazero/blob/ecac3f529b8db1dae13a95933a0aa15b0ccdf530/internal/syscallfs/syscall_windows.go#L109) side of it.

If no error translation is necessary for `Seek`, I think this is enough (but have no way to test it other than make the change and get your CI to do it):
```go
func maybeWrapFile(f file) file {
	return struct {
		readFile
		io.Writer
		syncer
	}{f, &windowsWriter{f}, f}
}

type syncer interface{ Sync() error }
```
ncruces: I had to rebase on head to try to fix Windows, which means I'm now broken for #1027.

Patch:
diff --git a/examples/basic/add.go b/examples/basic/add.go
index e1a0e06a..77c7b470 100644
--- a/examples/basic/add.go
+++ b/examples/basic/add.go
@@ -3,9 +3,10 @@ package main
 import (
 	"context"
 	_ "embed"
+	"errors"
+	"flag"
 	"fmt"
 	"log"
-	"os"
 	"strconv"
 
 	"github.com/tetratelabs/wazero"
@@ -25,6 +26,9 @@ var addWasm []byte
 // Since addWasm was compiled with TinyGo's `wasi` target, we need to configure
 // WASI host imports.
 func main() {
+	// Parse positional arguments.
+	flag.Parse()
+
 	// Choose the context to use for function calls.
 	ctx := context.Background()
 
@@ -40,31 +44,39 @@ func main() {
 	// function, implemented in WebAssembly.
 	mod, err := r.InstantiateModuleFromBinary(ctx, addWasm)
 	if err != nil {
-		log.Panicln(err)
+		log.Panicf("failed to instantiate module: %v", err)
 	}
 
 	// Read two args to add.
-	x, y := readTwoArgs()
+	x, y, err := readTwoArgs(flag.Arg(0), flag.Arg(1))
+	if err != nil {
+		log.Panicf("failed to read arguments: %v", err)
+	}
 
 	// Call the `add` function and print the results to the console.
 	add := mod.ExportedFunction("add")
 	results, err := add.Call(ctx, x, y)
 	if err != nil {
-		log.Panicln(err)
+		log.Panicf("failed to call add: %v", err)
 	}
 
 	fmt.Printf("%d + %d = %d\n", x, y, results[0])
 }
 
-func readTwoArgs() (uint64, uint64) {
-	x, err := strconv.ParseUint(os.Args[1], 10, 64)
+func readTwoArgs(xs, ys string) (uint64, uint64, error) {
+	if xs == "" || ys == "" {
+		return 0, 0, errors.New("must specify two command line arguments")
+	}
+
+	x, err := strconv.ParseUint(xs, 10, 64)
 	if err != nil {
-		log.Panicf("invalid arg %v: %v", os.Args[1], err)
+		return 0, 0, fmt.Errorf("argument X: %v", err)
 	}
 
-	y, err := strconv.ParseUint(os.Args[2], 10, 64)
+	y, err := strconv.ParseUint(ys, 10, 64)
 	if err != nil {
-		log.Panicf("invalid arg %v: %v", os.Args[2], err)
+		return 0, 0, fmt.Errorf("argument Y: %v", err)
 	}
-	return x, y
+
+	return x, y, nil
 }
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index 2313efa9..c966aaa5 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -950,7 +950,24 @@ func fdSeekFn(_ context.Context, mod api.Module, params []uint64) Errno {
 // and metadata of a file to disk.
 //
 // See https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#-fd_syncfd-fd---errno
-var fdSync = stubFunction(FdSyncName, []api.ValueType{i32}, "fd")
+var fdSync = newHostFunc(FdSyncName, fdSyncFn, []api.ValueType{i32}, "fd")
+
+func fdSyncFn(_ context.Context, mod api.Module, params []uint64) Errno {
+	fsc := mod.(*wasm.CallContext).Sys.FS()
+	fd := uint32(params[0])
+
+	type syncer interface{ Sync() error }
+	// Check to see if the file descriptor is available
+	if f, ok := fsc.LookupFile(fd); !ok {
+		return ErrnoBadf
+		// fs.FS doesn't declare Sync, but implementations such as os.File implement it.
+	} else if syncer, ok := f.File.(syncer); !ok {
+		return ErrnoBadf
+	} else if err := syncer.Sync(); err != nil {
+		return ErrnoIo
+	}
+	return ErrnoSuccess
+}
 
 // fdTell is the WASI function named FdTellName which returns the current
 // offset of a file descriptor.
diff --git a/internal/syscallfs/syscall_windows.go b/internal/syscallfs/syscall_windows.go
index ecfb3edb..52a46751 100644
--- a/internal/syscallfs/syscall_windows.go
+++ b/internal/syscallfs/syscall_windows.go
@@ -97,7 +97,8 @@ func maybeWrapFile(f file) file {
 	return struct {
 		readFile
 		io.Writer
-	}{f, &windowsWriter{f}}
+		syncer
+	}{f, &windowsWriter{f}, f}
 }
 
 // windowsWriter translates error codes not mapped properly by Go.
diff --git a/internal/syscallfs/syscallfs.go b/internal/syscallfs/syscallfs.go
index 13e40a66..195ff697 100644
--- a/internal/syscallfs/syscallfs.go
+++ b/internal/syscallfs/syscallfs.go
@@ -142,4 +142,7 @@ type readFile interface {
 type file interface {
 	readFile
 	io.Writer
+	syncer
 }
+
+type syncer interface{ Sync() error }


Test patch:
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index bfc46387..41e5518d 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -1666,13 +1666,48 @@ func Test_fdSeek_Errors(t *testing.T) {
 	}
 }
 
-// Test_fdSync only tests it is stubbed for GrainLang per #271
+// Test_fdSync only tests that the call succeeds; it's hard to test its effectiveness.
 func Test_fdSync(t *testing.T) {
-	log := requireErrnoNosys(t, FdSyncName, 0)
-	require.Equal(t, `
---> wasi_snapshot_preview1.fd_sync(fd=0)
-<-- errno=ENOSYS
-`, log)
+	tmpDir := t.TempDir() // open before loop to ensure no locking problems.
+	pathName := "test_path"
+	mod, fd, log, r := requireOpenFile(t, tmpDir, pathName, []byte{}, false)
+	defer r.Close(testCtx)
+
+	tests := []struct {
+		name          string
+		fd            uint32
+		expectedErrno Errno
+		expectedLog   string
+	}{
+		{
+			name:          "invalid fd",
+			fd:            42, // arbitrary invalid fd
+			expectedErrno: ErrnoBadf,
+			expectedLog: `
+==> wasi_snapshot_preview1.fd_sync(fd=42)
+<== errno=EBADF
+`,
+		},
+		{
+			name:          "valid fd",
+			fd:            fd,
+			expectedErrno: ErrnoSuccess,
+			expectedLog: `
+==> wasi_snapshot_preview1.fd_sync(fd=4)
+<== errno=ESUCCESS
+`,
+		},
+	}
+
+	for _, tt := range tests {
+		tc := tt
+		t.Run(tc.name, func(t *testing.T) {
+			defer log.Reset()
+
+			requireErrno(t, tc.expectedErrno, mod, FdSyncName, uint64(tc.fd))
+			require.Equal(t, tc.expectedLog, "\n"+log.String())
+		})
+	}
 }
 
 // Test_fdTell only tests it is stubbed for GrainLang per #271