Use Result in idiomatic wrappers, mark most wrappers as unsafe fn

[newpavlov]

Closes: #4
Closes: #8

[sunfishcode]

I'll need to think about this more, but at first glance I like the idea.

Errno does appear in the API in one place that isn't a return type though, within the Event struct. It's a little unfortunate that this patch could make some users working with that struct need to fall back to the raw::* interfaces to work with it.

What would you think of:

• keep Errno as an alias for __wasi_errno_t, and all the Errno constants as aliases for the __WASI_E* constants following what's there now
• add a new enum ErrorCode with an arm for each Errno constant except ESUCCESS (possibly combining this and the previous bullet using a macro to avoid redundancy)
• We could have impl From etc. to convert between this ErrorCode and Errno.
• use Result<T, ErrorCode> in the function signatures

?

[newpavlov]

Errno does appear in the API in one place that isn't a return type though, within the Event struct.

Result<(), NonZeroU16> and u16 have exactly the same memory layout, so we could use Result for Event and cast *mut Event to *mut __wasi_event_t before calling the raw method.

add a new enum ErrorCode with an arm for each Errno constant except ESUCCESS (possibly combining this and the previous bullet using a macro to avoid redundancy)

Unfortunately converting from raw error code to Result<(), ErrorCode> will not be a zero cost operation. Since if I am not mistaken _ => ErrorCode::___NonExhaustive will not be a simple cast. I think using NonZeroU16 for error code is not a bad approach for such low-level code.

[sunfishcode]

Bringing in Vec and String makes me curious about the boundaries here. How do we decide what code belongs in this repo, versus in the Rust standard library?

The args and environ functions are low-level interfaces. The only reason I can think of why anyone would want to bypass the Rust standard library to call these would be if they really want to avoid any dynamic allocation, but in that case these Vec/String interfaces aren't appropriate either.

I'm also curious; if we do add these interfaces, will the Rust standard library will need to do its own dynamic allocation anyway, to copy the data into OsStrings?

[newpavlov]

Well, if you really don't want any allocations, there is still the raw interface. I guess we could reduce amount of allocations to just two by introducing a custom iterator, which will store the argv buffer.

An alternative would be to write something likes this:

fn get_args(f: impl FnMut(value: &[u8])) -> Result<(), Error> { .. }

In C we can use alloca to work around dynamic sizes, IIUC we could use unsized rvalues, but we still can't return anything unsized. Although with the callback approach unsized rvalus will allow us to make this function allocations-free. But even if this feature was implemented I am not sure if we want to make this crate nightly-only.

will the Rust standard library will need to do its own dynamic allocation anyway, to copy the data into OsStrings

Yes, indeed in the current form this function will increate a total amount of allocations, so maybe idea with the custom iterator or callback is worth implementing.

[newpavlov]

BTW does WASI provide any guarantees about strings encoding for args and env?

[sunfishcode]

I guess the question is, what's the motivating use case here? We do already have a fine implementation of args() in the standard library.

Currently, WASI does not define the encodings. I expect they'll be defined to be UTF-8 in the future, though we haven't had the time yet.

[newpavlov]

How about API completeness? :) I've changed get_args (and get_environ) to a callback-based approach, so std implementation using it will have exactly the same number of allocations and in future we will be able to completely remove allocations from them.

[newpavlov]

I have reworked env and args a bit, now API allows to pre-allocate buffer for them.

[newpavlov]

@sunfishcode
Hi! Any updates on this?

[sunfishcode]

Ok, this looks good to me, with just a few more minor comments.

I'm still a little unclear on the relationship between the args/environ code here and Rust libstd, but we can figure that out on the libstd side.

[newpavlov]

I marked wrappers as unsafe. I also removed __wasi_proc_raise, since IIRC you plan to remove it from Core API.

[sunfishcode]

Sounds good!

[newpavlov]

Great! Do you plan to publish v0.6 soon?

[sunfishcode]

Yes, I just published this in 0.7 (I published 0.6.0 for the license change).