-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Ignoring the formatting issue that Travis chokes on, this looks good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a complex merge, with the source path moving a bunch of things around, and the destination patch having some conflicting changes! It looks good overall.
README.md
Outdated
@@ -25,28 +25,47 @@ Please note that the library requires Rust compiler version at least 1.34.0. | |||
We support a subset of the [WASI API], though we are working on new hostcalls | |||
on a regular basis. We currently implement: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is all the remaining host calls, can we drop the "subset" here now? Also, can we drop the list of functions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but we're still missing these:
proc_raise
sock_recv
sock_send
sock_shutdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. In that case, maybe we can just say we support everything except those, and annotate them:
proc_raise
- we expect this will be removed from WASI, as it isn't very useful without full signal handling supportsock_recv
,sock_send
,sock_shutdown
- networking support is not implemented yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you go about implementing proc_raise
in a generic way? Setting an actual signal handler is not really an option in runtimes that don't run wasm modules in a dedicated process.
I didn't implement the sock_*()
hostcalls since there's not much that can be done with them yet. We can't accept() a client, we can't connect(), we can't get any information about a socket. So, even as a stopgap until we come up with a wasi.net
module, this set of functions doesn't seem to be very useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned below, I've put the sock_
hostcalls into a separate submodule hostcalls/sock.rs
with unimplemented!
stubs in them. I reckon this submodule can act as a placeholder until wasi.net
lands.
In terms of proc_raise
, if you guys want, I can chuck it out altogether. Lemme know what's your preference on that front! :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've now put two things in the README:
- that we support all hostcalls with the expection of sockets; but we cannot do anything about this until
wasi.net
lands - we do not support
proc_raise
as it is expected to be dropped from WASI altogether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
src/hostcalls.rs
Outdated
.map(|_| wasm32::__WASI_ESUCCESS) | ||
.unwrap_or_else(|e| e) | ||
} | ||
|
||
#[wasi_common_cbindgen] | ||
pub fn fd_prestat_get( | ||
pub fn fd_pwrite( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Lucet patch, hostcalls.rs was split into hostcalls/fs.rs hostcalls/misc.rs. That seems a desirable direction to head in, both for general code maintenance, and since at the WASI Subgroup level there seems to be broad support for splitting filesystem and other parts of the API into separate modules. Would it be difficult to do a similar split here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reorganised the hostcalls into submodules like in lucet-wasi. In terms of missing sock_
hostcalls, I've put them into a submodule of their own sock.rs
.
src/hostcalls.rs
Outdated
) -> wasm32::__wasi_errno_t { | ||
unimplemented!("fd_filestat_set_size") | ||
let fd = dec_fd(fd); | ||
// TODO: is this the correct right for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this TODO comment was removed in the Lucet patch, but I actually think it's something I'd like to think about more, so I'm actually happy you left it in :-).
Looks great! |
I've tried my best to import and merge all changes from lucet-wasi which landed with bytecodealliance/lucet#176. There is still some unsafe-ness left that could probably be taken care of (see here).
I'd be indebted if you could proofread it all! :-)