new: declare type wasi_ctx in c-api

[gaukas]

This pull request incompletely exports WasiCtx in C-API:

• Declare wasi_ctx_t in wasi.h.
• Add implementation for wasi_ctx_insert_file and wasi_ctx_push_file in wasi.rs.
• Add implementation for wasmtime_context_set_default_wasi_if_not_exist and wasmtime_context_get_wasi_ctx in store.rs.

Discussed in bytecodealliance/wasmtime-go#187 with @alexcrichton.

The corresponding Go changes will be available in a separate pull request to wasmtime-go.

Please let me know if there are any issues or suggested edits.

[gaukas]

Fixing Rustfmt... [DONE]

[pchickey]

I missed the discussion in bytecodealliance/wasmtime-go#187 and I am a little nervous about these changes - they can be supported today, but will have to change substantially in the future.

In our preview 2 implementation, we have eliminated all of the user-facing methods on WasiCtx. This is required due to new invariants that we need to maintain for preview 2 resources. You can still insert (host filesystem) directories into a WasiCtx while in the builder. We also eliminated the WasiFile/WasiDir traits as a mechanism for extending the Wasi implementation. We do have extension traits for implementing streams (which maybe will be what you need, to expose pipes?) and a totally new interface and implementation for sockets. The story for how to expose these new interfaces to guests is itself a bit involved as well - we have adapters to transition modules consuming just Wasi preview 1, but it sounds like you are using preview 1 plus some other imports as well.

We expect the preview 2 builder/context (defined in wasmtime-wasi::preview2) will supplant the preview 1 implementation (defined in wasi-common) pretty soon, and preview 1 will be supported through the p2->p1 host compatibility layer (wasmtime-wasi::preview2::preview1). We will be supporting both the new and old Wasi implementations a little while for users to transition, but that is much easier to manage in Rust than in the C API. The existing C APIs which end up connecting to wasi-common are pretty easy to transition over wasmtime-wasi::preview2, but these methods will not be possible to transition.

One way we can bypass the many wrinkles in this story is if we can switch you over to using Wasi Preview 2 and the Component Model. That still has open questions about how to use the C FFI, but those are questions that it will benefit a lot of users to solve, whereas if we expose these methods for just you to use today, we'll also have to solve other problems for just you in a few months when it is time to transition to preview 2.

This is a pretty complex design space which doesn't have much by the way of docs, so if I can help you understand this problem better by discussing it, we can talk on the bytecode alliance zulip or setup a call sometime.

[gaukas]

I understand your concern. The reason for this PR to exist is simply that the Go's implementation of wasmtime (as wasmtime-go) has been missing way too many features from its Rust counterpart, and we really need BOTH to work on the same page. There's a set of projects we are working on right now and part of its job can ONLY be done in Go. However the gap between two implementation due to the absence of relevant C-API is irritating, and it feels like it is never a part of plan to allow the users with Go to do as much as Rust users can. If so, it is pretty sad, as I believe Go has its significance as much as Rust do.

It is not a bad idea to switch over to wasi preview2, but I am concerned about the amount of work needed to fill the gap in wasmtime-go, given it looks like not as popular and already fell short on features in preview 1. In the long run I believe it is definitely worth considering, though, if it implements a much more consistent feature set in across all ports. Specifically, the ability to inject sockets AFTER instance has been created/running.

A discussion will be nice to have, we can start by setting up a covert communication channel. Please feel free to shoot me an email if you wish.

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[pchickey]

Wasmtime's C FFI isn't fully fleshed out, not because we don't want it to be, but because we have had other priorities. We are happy to help contributors add C FFI support for the parts of wasmtime where it is missing right now.

[alexcrichton]

Thanks for the PR! I've noted a few issues below, but @pchickey I think the functionality added here should work both before and after preview2. This is dealing with concrete system primitives as opposed to user-defined things, so it seems reasonable to be able to add/insert those after-the-fact both during the building process and once the context is created too.

We'll definitely need one API for preview2 and one for preview1 (or somehow update the existing one for preview2), but I think with the various issues below addressed I'd personally be ok merging this.

[pchickey]

I looked at this again and talked to @alexcrichton about how we can move this over to preview 2 and I'm comfortable with it once he signs it off.

[alexcrichton]

Thanks! I'm remembering now that there's other issues related to portability which this will need to handle too

[alexcrichton]

I was looking at host_fd here again and first though "hm shouldn't this be int?" since that matches what the host has here, but then I remembered additionally that this won't work on Windows because Windows doesn't have file descriptors.

I think that this and the below function will need to use a different type for host_fd on Windows where it'll use int on Unix and HANDLE on Windows.

[alexcrichton]

Also, could this document the access_mode values? I don't believe that maps to octal permissions like 0o777 as one might otherwise expect.

[gaukas]

access_mode values

This is basically the same as whatever in Rust.

[gaukas]

I think that this and the below function will need to use a different type for host_fd on Windows where it'll use int on Unix and HANDLE on Windows.

Would you give an example or directly push to my branch? I do not have a Windows setup for testing any change specific to Windows. Or, if you could point me to a more detailed setup guide for developing Wasmtime on Windows. (Especially for the wasmtime-go)

But again, I'm not at all familiar with Windows types/APIs. Possibly needs someone better than me to do it for the thorough testing procedure.

[gaukas]

it'll use int on Unix

Seems even 'int' is not the case to me. It seems to be i32 on Unix.

Opt to use u32 here simply for the type consistency across all fds -- iirc the returned guest_fd is in u32. I can double check.

[gaukas]

And IMO, it is Rust's fault to select i32 for file descriptors. An apparently better and more consistent choice is uintptr.

I'm personally against exposing host_fd as i32, but if you insist it should guarantee truthfulness (host_fd: i32, guest_fd: u32) rather than consistency, I can make the corresponding change.

[gaukas]

I looked again and found Go will only return uintptr for the fd.

I would propose the change to always use * c_void(Rust)/void * for all guest/host fd for the interface, and leaving the type conversion inside Rust implementation. What do you think? @alexcrichton

Btw I am not so sure if the std::os::fd::FromRawFd would work at all on Windows platforms. It says: This module is supported on Unix platforms and WASI, which both use a similar file descriptor system for referencing OS resources.
Seems it should be FromRawHandle on Windows.

[alexcrichton]

Sorry I don't have detailed guides and such of how to build on Windows, I also don't develop on Windows myself. This problem must be fixed to merge this PR, however, because I believe your changes will not compile on Windows due to the usage of std::os::fd which does not on Windows. CI is passing because only a subset of tests are run for PRs, but the full merge will run a full suite of tests. If you'd like you can include a commit with prtest:full somewhere in the commit message to run full CI on this PR.

As for types, IMO this should match what happens in the native platform. This means that int should be used in the header file for Unix and HANDLE should be used in the header file for Windows. The corresponding Rust types are std::os::fd::RawFd and std::os::windows::io::RawHandle.

I don't have a better idea of how to define this function other than to just have it defined differently on different platforms.

[gaukas]

Luckily it worked after some reverse engineering of the CI script of wasmtime and wasmtime-go, now I can finally compile and test on Windows.

Just pushed a new change to use void*/*mut c_void/unsafe.Pointer() for host_fd. How does it look to you? Or would you still want different function header on different platforms. (In that case, could you please provide some guidance on how to conditionally declare function header on different platforms?)

[alexcrichton]

Personally I don't think that void* should be used here as a placeholder for HANDLE-or-int. I realize this is what Go seems to do with uintptr as the return value from the Fd() method, but that's not how I'd personally do it. That being said I also don't have a lot of prior experience to draw from. Everything in Rust is "just use File" which isn't an option in C and otherwise I'm not aware of any C libraries that handle the Windows/Unix distinction as they generally take an fd and then don't work on Windows. To me though casting int in C to void* just to pass it to this function feels a bit too weird.

Otherwise though another comment I would have is that #[cfg] can be applied on a much more granular level than the entire function, so I'd recommend avoiding duplication of the entire function bodies in Rust with something like:

#[cfg(unix)]
let file = File::from_raw_fd(the_argument);
#[cfg(windows)]
let file = File::from_raw_handle(the_argumet);

(or something like that)

[gaukas]

So do we have a conclusion here, shall we include multiple functions for Unix and Windows to have explicit types (int32/i32 and void*/HANDLE), or do you think it is okay to save them for later and keep it as you described

#[cfg(unix)]
let file = File::from_raw_fd(the_argument);
#[cfg(windows)]
let file = File::from_raw_handle(the_argumet);

[alexcrichton]

All I can really offer here is my own personal opinion and preference. My preference is that the types of the argument reflect what the system uses, which in C is int on Unix (not int32_t) and HANDLE on Windows. The Rust version of those is Raw* in the standard library. I don't have a strong preference on whether there's two different names for the function, but I think it's reasonable to have the same symbol with a platform-specific signature, meaning just one function with different types depending on the platform.

[gaukas]

Now using RawFd and RawHandle.

On C's side:

Unix:

WASM_API_EXTERN void wasmtime_context_insert_file(wasmtime_context_t *context, uint32_t guest_fd, int host_fd, uint32_t access_mode);
WASM_API_EXTERN wasmtime_error_t *wasmtime_context_push_file(wasmtime_context_t *context, int host_fd, uint32_t access_mode, uint32_t *guest_fd);

Windows:

WASM_API_EXTERN void wasmtime_context_insert_file(wasmtime_context_t *context, uint32_t guest_fd, void *host_handle, uint32_t access_mode);
WASM_API_EXTERN wasmtime_error_t *wasmtime_context_push_file(wasmtime_context_t *context, void *host_handle, uint32_t access_mode, uint32_t *guest_fd);

Tried to use HANDLE from <winnt.h> but it turns out CGO won't even work, throws a bunch of errors such as

error: unknown type name 'DWORD'
error: unknown type name 'WORD'

Defining typedef void *HANDLE in store.h didn't work out for me either, I failed to figure out a way to convert the uintptr into the self-defined C.HANDLE type. Casting into C.HANDLE() simply does not work. Therefore, here I choose to stick with void* in C-API, since it is just its true identity. Please feel free to push any further changes, but I guess that will be all I have for this.

[alexcrichton]

Could this choose names such as WASMTIME_UNIX or WASMTIME_WINDOWS to avoid clashing with other projects?

[gaukas]

Sure, you may just push a change for styling suggestions like that...

[alexcrichton]

Could this perhaps do

#ifdef WASMTIME_UNIX
typedef int wasmtime_host_file_t;
#else   
typedef HANDLE wasmtime_host_file_t;
#endif 

to avoid defining the function signature twice?

Additionally this should use HANDLE on Windows, not void*, as fixing that in Go seems like an orthogonal concern?

[gaukas]

I'm not really getting it by what do you mean here:

#ifdef WASMTIME_UNIX
typedef int wasmtime_host_file_t;
#else   
typedef HANDLE wasmtime_host_file_t;
#endif 

Doesn't this just further complicates the interface by adding more layers of indirectness?

Go seems like an orthogonal concern

To be super honest with you, for me, my major concern is if it works in Go. So I would just save the effort. If it won't work for you, someone better at this could fix it...

[alexcrichton]

I think the documentation here and below may wish to be updated where these constants are not defined by the WASI spec, they're purely an implementation detail in Wasmtime.

Could this additionally split out the read/write to WASMTIME_WASI_FILE_{READ,WRITE} perhaps as #defines?

[gaukas]

not defined by the WASI spec

Then possibly some better documentation is needed on the wasi_common package instead. I found no indication saying this is a pure wasmtime thing.

[alexcrichton]

Like with C, can the #[cfg] be used to conditionally define a type and delegate to a small helper internally? As I mentioned before that would avoid duplicating the whole function.

[gaukas]

I don't think so since we have different input type. Unless you want to eliminate that? I didn't find a good way to conditionally define ONLY the func header without further complicating the API/code.

[gaukas]

I guess it is time for someone from bytecodealliance to deal with the PR, if you want to make a change, do it. I allowed changes by maintainers.

I already got bored of this low communication efficiency and have much more prioritized work to do. And all I wanted could already be achieved with my fork, and for code style issue, please, no one can reach the agreement with others so push changes rather than suggesting one. I'm pissed and I hope I made that obvious.

[gaukas]

[alexcrichton]

Ok thanks for the initial effort here. I'm going to close this and others can pick this up if they're interseted. If you feel inspired in the future feel free to reopen and/or resubmit as well.