Begin sketching out a new high-level `fs` API. #91

sunfishcode · 2019-09-10T04:50:25Z

This is a very preliminary sketch of #83. It doesn't even compile yet, but it shows a possible high-level structure of such an API.

kubkon · 2019-09-10T09:40:29Z

src/fs/ctx.rs

+use crate::WasiCtx;
+
+lazy_static! {
+    // TODO: Should we allow the context to be passed alternate arguments?


Hmm, I guess we should. How otherwise are we going to pass in the preopens? Or are we going to add another mid-level call for that perhaps?

Thinking about this a little more, I don't think a lazy_static singleton is the way to go, because people using this API may want to have multiple distinct WasiCtxs that are isolated from each other. I've now updated the patch to have Dir and File hold a reference to a WasiCtx. We may further want to make it an Rc, but we'll see how things go.

Good point!

sunfishcode · 2019-09-11T07:38:34Z

This is now complete enough to compile, and a lot of the API surface area is now in place, though most of the functionality isn't implemented yet.

kubkon · 2019-09-11T08:44:40Z

src/fs/dir.rs

+        ))?;
+        */
+
+        let ctx = self.ctx;


Is this necessary here? Could we not simply pass self.ctx directly?

kubkon · 2019-09-11T08:47:49Z

src/fs/error.rs

+    Err(io::Error::from_raw_os_error(raw_os_error))
+}
+
+#[cfg(windows)]


Do you think that in the future it'd be useful to have the same setup as for the hostcalls? That is, separate host specifics in modules and then just do a cfg_if instead of multiple #[cfg(..)]s? Mind you, this is an idea for the future and not now; I'm happy with the state of error.rs as is :-)

kubkon · 2019-09-11T08:48:16Z

src/fs/metadata.rs

+///
+/// [`std::fs::Metadata`]: https://doc.rust-lang.org/std/fs/struct.Metadata.html
+#[derive(Clone)]
+pub struct Metadata {}


Suggested change

pub struct Metadata {}

pub struct Metadata;

kubkon

Looks great! Here's a thought: could we actually re-use this interface and the implementation in Rust's libstd? I might be way off base here but browsing through the changes here looks as if it could be re-used in libstd. @alexcrichton thoughts?

alexcrichton · 2019-09-11T14:17:26Z

Seems possible perhaps! I suspect this'd want to bake a bit more before going into libstd, but the internal details of libstd are up for grabs to change at any time!

sunfishcode · 2019-09-11T16:49:36Z

Interesting idea. In addition to obviously baking :-), a few additional pieces we'd need:

A way for this fs API to use WASI directly, instead of going through the current wasi-common userspace implementation. Some of that may be as simple as just adding alternate implementations of the API under #[cfg(target_os = wasi)]. What should we do about WasiCtx though? In wasi-common, one can create multiple independent WasiCtxs and they're explicitly passed into the API. In WASI itself, there's currently just one implicit WasiCtx (or equivalent) below the platform boundary. One option is to change WASI itself to have a context argument and support independent contexts, I suppose.
Something similar to what libpreopen does within WASI libc, so that when Rust code calls File::open("/absolute/path"), we can remap it to directory fd and relative path. This should be straight-forward.
"Rust-idiomatic" wrappers on top of wasi-common's low-level API, similar to the wasi crate, using Results and slices rather than bare errnos and pointers. This is what the Rust libstd support for WASI currently uses, and it would be nice if this new fs layer could use something similar.

@cc newpavlov since this is one possible path to a libc-free libstd for WASI targets.

sunfishcode · 2019-09-11T23:45:46Z

cc @newpavlov even, since this is one possible path to a libc-free libstd for WASI targets.

newpavlov · 2019-09-12T10:55:49Z

this is one possible path to a libc-free libstd for WASI targets.

I am not sure if I understand how it will work. All WASI code still works via Core API at the lowest level, right? And this high-level API is still built on top of Core API?

Right now, we depend on libc in std only for interoperability with C/C++. Removing the remaining libc usages should be fairly trivial if we'll add a separate target (e.g. wasm32-wasi-rust).

Not strictly relevant, but can you explain how __wasilibc_find_relpath works in terms of Core API? I am not completely sure, but it looks like we can remove its usage from std.

This is a very preliminary sketch of #83. It doesn't even compile yet, but it shows a possible high-level structure of such an API.

sunfishcode · 2019-09-30T17:05:04Z

This builds, and although most of the functionality is not yet implemented, I'd like to land it and continue to iterate incrementally.

kubkon · 2019-09-30T17:20:05Z

@sunfishcode sounds great to me! Feel free to merge it or I can do that if you want. Also, apologies for little to no activity lately but got swamped with preparation work for the upcoming Devcon5 conference :-)

kubkon reviewed Sep 10, 2019

View reviewed changes

sunfishcode force-pushed the fs branch 5 times, most recently from 78132c9 to 522a9fc Compare September 11, 2019 03:55

kubkon reviewed Sep 11, 2019

View reviewed changes

src/fs/dir.rs

))?;

*/

let ctx = self.ctx;

Copy link

Member

kubkon Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary here? Could we not simply pass self.ctx directly?

kubkon reviewed Sep 11, 2019

View reviewed changes

kubkon approved these changes Sep 11, 2019

View reviewed changes

kubkon force-pushed the fs branch from 522a9fc to 1781b79 Compare September 11, 2019 12:04

sunfishcode added 8 commits September 29, 2019 11:53

Begin sketching out a new high-level fs API.

1c55514

This is a very preliminary sketch of #83. It doesn't even compile yet, but it shows a possible high-level structure of such an API.

Stub out more functionality.

8ce1cef

Switch from a lazy_static WasiCtx to a borrowed one.

514d7f2

Reformat some comments.

d63a0cd

Code-quote Self.

240f7cc

Implement error translation for Windows.

d9531ef

Calls to fd_close are now unsafe.

847d9ed

Implement a few more functions.

4614fbc

sunfishcode force-pushed the fs branch from 1781b79 to 4614fbc Compare September 30, 2019 17:03

sunfishcode marked this pull request as ready for review September 30, 2019 17:03

Merge branch 'master' into fs

e8fa7f6

sunfishcode merged commit 6b2f3eb into master Sep 30, 2019

sunfishcode deleted the fs branch September 30, 2019 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Begin sketching out a new high-level `fs` API. #91

Begin sketching out a new high-level `fs` API. #91

sunfishcode commented Sep 10, 2019

kubkon Sep 10, 2019

sunfishcode Sep 10, 2019

kubkon Sep 11, 2019

sunfishcode commented Sep 11, 2019

kubkon Sep 11, 2019

kubkon Sep 11, 2019

kubkon Sep 11, 2019

kubkon left a comment

alexcrichton commented Sep 11, 2019

sunfishcode commented Sep 11, 2019

sunfishcode commented Sep 11, 2019

newpavlov commented Sep 12, 2019

sunfishcode commented Sep 30, 2019

kubkon commented Sep 30, 2019

Begin sketching out a new high-level fs API. #91

Begin sketching out a new high-level fs API. #91

Conversation

sunfishcode commented Sep 10, 2019

kubkon Sep 10, 2019

Choose a reason for hiding this comment

sunfishcode Sep 10, 2019

Choose a reason for hiding this comment

kubkon Sep 11, 2019

Choose a reason for hiding this comment

sunfishcode commented Sep 11, 2019

kubkon Sep 11, 2019

Choose a reason for hiding this comment

kubkon Sep 11, 2019

Choose a reason for hiding this comment

kubkon Sep 11, 2019

Choose a reason for hiding this comment

kubkon left a comment

Choose a reason for hiding this comment

alexcrichton commented Sep 11, 2019

sunfishcode commented Sep 11, 2019

sunfishcode commented Sep 11, 2019

newpavlov commented Sep 12, 2019

sunfishcode commented Sep 30, 2019

kubkon commented Sep 30, 2019

Begin sketching out a new high-level `fs` API. #91

Begin sketching out a new high-level `fs` API. #91