WASI: `fs.FS` as Preopen parameter with implementation of `fd_prestat_get` #362

nullpo-head · 2022-03-11T17:40:05Z

This PR changes the input type of preopen from wasi.FS to fs.FS, which is more natural for users.
This PR also includes implementation of FdPrestatGet and PathOpen flags, which are necessary for multiple pre-open directories to work correctly.

internal/wasi/fs.go

mathetake · 2022-03-17T03:04:50Z

internal/wasi/fs.go

+	for name, entry := range dir.Entries {
+		entries = append(entries, &memFileInfo{name: path.Base(name), fsEntry: entry})
+	}
+	sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() })


q: is sorting dirs necessary?

yes, it's necessary. fs.ReadDir requires the result is sorted, and the `fstest.TestFS" becomes flakey if we don't sort.

internal/wasi/fs.go

internal/wasi/wasi.go

mathetake · 2022-03-17T03:07:30Z

internal/wasi/wasi.go

+		return wasi.ErrnoBadf
+	}
+	writer, ok := f.file.(io.Writer)
+	if !ok {


q: in what case !ok in real world?

Realworld examples are when the fily system of f.file is an embed.FS. f.file is fs.File, which doesn't require io.Writer, so it doesn't always implement io.Writer. Maybe it's better to comment that. let me do it

internal/wasi/wasi.go

codefromthecrypt

Thanks for getting this started. I think there will be a fair amount of work to finish this up, and we don't want to hold the supporting changes hostage through that. Can you extract the wasi api changes and tests into a separate PR from the fs.FS stuff?

README.md

internal/wasi/fs.go

codefromthecrypt · 2022-03-17T03:32:33Z

internal/wasi/fs_test.go

+		"simple file": {Contents: []byte("simple cont")},
+		"directory": {
+			Entries: map[string]*memFSEntry{
+				"file1 inside directory": {Contents: []byte("simple cont file1 inside directory")},


maybe use more realistic paths here? it would make it easier to read and know the intent.

ex would it be "directory/file1" because this includes the name of the key?

also the contents if dummy can be shorter like single character or single word

I've changed the names to more realistic paths, but let me know if those are not what you intended. THanks!

internal/wasi/fs_test.go

codefromthecrypt · 2022-03-17T04:36:49Z

I'm going to take over this PR for some several minutes, rebase it after extracting the WASI functions to another PR then force push a more isolated variant to proceed with. bear with me a bit

codefromthecrypt · 2022-03-17T05:34:52Z

once #388 is merged I'll rebase this and hand it back over to @nullpo-head. hopefully next time we can keep fixing WASI APIs separate from new API work that uses them.

…ssary PathOpen features. Signed-off-by: Takaya Saeki <[email protected]>

codefromthecrypt · 2022-03-17T05:56:00Z

okie dokie over to you!

Signed-off-by: Takaya Saeki <[email protected]>

nullpo-head · 2022-03-17T09:31:32Z

ptal @codefromthecrypt @mathetake thanks!

codefromthecrypt

memFS looks suspiciously like fstest.MapFS Please delete it and use fstest.MapFS if you need a dummy filesystem besides embedFS. The main point of this change is to stop bespoke APIs and use fs.FS instead.

I would be very very surprised if there isn't an existing fs.FS library out there that supports writes.

If we couple OpenFile(path string, flag int, perm os.FileMode) (fs.File, error) with fs.FS we almost guarantee people can't bring their own impls as an existing impl wouldn't be using our interfaces. Instead, until we find an alternative to OpenFile (this is a TODO to find one!), make a separate Config option for a write path that accepts this interface.

ex. config.WithOpenFile(path, OpenFile) and put a TODO to look more into options for writes that don't imply completely custom implementations of fs.FS

codefromthecrypt · 2022-03-17T11:25:56Z

examples/file_system_test.go

+	// Configure what resources you share with the WASI program.
+	wasiConfig := wazero.NewWASIConfig()
+	// Share fs.FS as pre-opened directories
+	wasiConfig = wasiConfig.WithPreopens(


Feels like WithXXXMount will be easier for people to understand that "preopens" WDYT? Also, we can make it chained instead of a map. the map can be an internal detail

Ex.

// documented as read-only wasiConfig.WithMount("/input", embeddedFS)

codefromthecrypt · 2022-03-17T11:53:41Z

examples/file_system_test.go

+		},
+	)
+	// the wasm module will concatenate the contents of the two input.txt and write to output.txt
+	wasiConfig = wasiConfig.WithArgs("./input.txt", "/input/input.txt", "./output.txt")


just overwrite a file instead? that's a lot simpler and less code.

codefromthecrypt · 2022-03-17T12:19:09Z

wasi/wasi.go

+	// OpenFile opens the named file with specified flag in the same way as os.OpenFile.
+	// For example, if the path does not exist and flag includes os.O_CREATE, it will create a new file with perm.
+	//
+	// Unlike os.OpenFile, OpenFile must reject paths that do not satisfy `fs.ValidPath(path)`,


I have a hard time believing there are any strict requirements that need to be made here based on the WASI spec. It doesn't have spec tests and at best very vaguely documented. Any constraints that make something behave different than something else in Go should be not just mentioned but also answered why. (Ex in RATIONALE is a good place as there's not likely any spec or spectest to say why)

Can you please document where these are coming from?

codefromthecrypt · 2022-03-17T23:43:10Z

I'm going to fork this and try to salvage what I can from it, but generally this is far from ideal and time is better spent on another path. Hence closing it.

codefromthecrypt · 2022-03-17T23:55:13Z

internal/wasi/wasi.go

 func (c *Config) Stdin(reader io.Reader) {
-	c.stdin = reader


not currently planning to do anything to try to make stdin/stdout/stderr act like normal files as they aren't and it is more distracting forcing them though a struct that only has one side useful than the special casing in the first place.

codefromthecrypt · 2022-03-18T00:21:46Z

There's nothing to salvage from this PR except experience. Once things are stable we can do another impl, but research and citation here are a prerequisite so that we don't fall into the same hole again #390

Thanks for giving this a try

codefromthecrypt · 2022-03-20T06:32:08Z

internal/wasi/fs.go

+	// fs.FS requires a FS to reject a path that doesn't satisfy fs.ValidPath.
+	if !fs.ValidPath(name) ||
+		// '\' works as alternative path separater and ':' allows to express a root drive directory.
+		// fs.FS implementation must reject those path on Windows. See Note in the doc of fs.ValidPath.


this is a good example of a leaky abstraction. WASI isn't and OS and if it were it wouldn't be windows, and there's nothing about fs.FS that should ever allow windows paths to leak into it. Ex. if someone exposes their "c:" drive, that's not on us to enforce. The only sensible file mappings would be chrooted if even an actual filesystem.

In other words windows-based file handing in a barely specified posix system layer is a non-goal. We can use this to simplify discussions in future work. By adding code to address things like OS-specific things inside WASI, we are actually making the problem more confused, especially to people new who may not know Wasm is effectively a virtual machine, and if shares anything in common with OS, it would be unix, not windows, that we'd optimize for.

nullpo-head force-pushed the wasi_fdstat branch 5 times, most recently from b7e2e0e to ad2dc8a Compare March 16, 2022 18:39

nullpo-head changed the title ~~Wasi fdstat~~ WASI: fs.FS as Preopen parameter with implementation of fd_prestat_get Mar 16, 2022

nullpo-head marked this pull request as ready for review March 16, 2022 18:55

nullpo-head requested review from codefromthecrypt and mathetake March 17, 2022 01:38