Skip to content

Implement a WASI instantiation C API.#977

Merged
peterhuene merged 10 commits intobytecodealliance:masterfrom
peterhuene:wasi-c-api
Feb 26, 2020
Merged

Implement a WASI instantiation C API.#977
peterhuene merged 10 commits intobytecodealliance:masterfrom
peterhuene:wasi-c-api

Conversation

@peterhuene
Copy link
Copy Markdown
Member

This PR implements an initial WASI C API that can be used to instantiate
and configure a WASI instance from C.

This also implements a WasiBuilder for the C# API enabling .NET hosts to bind
to Wasmtime's WASI implementation.

Peter Huene added 6 commits February 24, 2020 17:42
This commit implements an initial WASI C API that can be used to instantiate
and configure a WASI instance from C.

This also implements a `WasiBuilder` for the C# API enabling .NET hosts to bind
to Wasmtime's WASI implementation.
Close the file descriptors before attempting to reopen the files.
Remove unnecessary loop when marshaling lists of strings as pointers to UTF-8
bytes.
Comment thread crates/c-api/Cargo.toml
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I'm not one really for reviewing much of the C#, but I can at least take a look at the Rust parts :)

Comment thread crates/c-api/src/wasi.rs
impl wasi_config_t {}

#[no_mangle]
pub unsafe extern "C" fn wasi_config_new() -> *mut wasi_config_t {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW for new APIs we may want to explore updating how we write bindings. For example we could make all functions safe ideally, and this function could, for example, return Box<wasi_config_t> instead of *mut which would avoid extra boilerplate.

Another example would be that wasi_config_delete would be safe and take a Box<wasi_config_t> as well perhaps. I've been meaning to see how far we can take this because it'd be great to avoid so much unsafety in the bindings, but it definitely breaks down at some point, for example when handling *const c_char

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That would be interesting to do. I don't like the unsafety of this layer either and while it needs to exist at some level to interact with C, perhaps we could make it much more difficult for us to introduce leaks and use-after-frees by generating the unsafe boilerplate.

Comment thread crates/c-api/src/wasi.rs Outdated
Comment thread crates/c-api/src/wasi.rs Outdated
Comment thread crates/c-api/src/wasi.rs Outdated
Comment thread crates/c-api/include/wasi.h Outdated
Copy link
Copy Markdown
Contributor

@yurydelendik yurydelendik left a comment

Choose a reason for hiding this comment

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

Reviewed C# code, bulk of it:using and != null reorg, Binding and Host refactoring, better Disposing. Looks good 👍

FWIW agree with comment about mem::forget

Comment thread crates/c-api/src/wasi.rs
Peter Huene added 2 commits February 25, 2020 15:41
This commit makes `WasiCtxBuilder` take `&mut Self` and return `&mut
Self` for its methods.  This is needed to allow for the same
(unmoved) `WasiCtxBuilder` to be used when building a WASI context.

Also fixes up the C API to remove the unnecessary `Box::from_raw` and
`forget` calls which were previously needed for the moving version of
`WasiCtxBuilder`.
This commit renames `wasi_config_set_std[in|out|err]` to
`wasi_config_set_std[in|out|err]_file` so we can reserve the former for
when the C API supports a stream abstraction.
@peterhuene peterhuene merged commit 78b32e2 into bytecodealliance:master Feb 26, 2020
@peterhuene peterhuene deleted the wasi-c-api branch February 26, 2020 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants