Improvements to WasiCtxBuilder, and a couple bug fixes
#175
Conversation
acfoltzer
force-pushed
the
acf/ctxbuilder
branch
2 times, most recently
from
f759503 to
b4fe288
Compare
|
I'm marking this as ready for review, since it doesn't look like bytecodealliance/wasmtime#490 will pass until we have a final revision to use in wasmtime. |
|
Per discussion with @sunfishcode, |
|
Got it! I thought |
| .args | ||
| .into_iter() | ||
| .map(|arg| arg.into_utf8_cstring()) | ||
| .collect::<Result<Vec<CString>>>()?; |
There was a problem hiding this comment.
This is not a nitpick, just an FYI that you could have omitted the concrete type stored inside the container here, i.e., Result<Vec<_>> :-)
| (Ok(k), Ok(v)) => Ok((k, v)), | ||
| _ => Err(Error::ENOTCAPABLE), | ||
| } | ||
| (k.as_ref().to_vec().into(), v.as_ref().to_vec().into()) |
| .args(args) | ||
| .inherit_stdio() | ||
| .inherit_env() | ||
| .build() |
There was a problem hiding this comment.
Yeah, from the code readability point of view, that's so much better. Well done!
This changes the fields on the builder to types that let the various `.arg()`, `.env()`, etc methods infallible, so we don't have to worry about handling any errors till we actually build. This reduces line noise when using a builder in a downstream application. Deferring the processing of the builder fields also has the advantage of eliminating the opening and closing of `/dev/null` for the default stdio file descriptors unless they're actually used by the resulting `WasiCtx`. Unicode errors when inheriting arguments and environment variables no longer cause a panic, but instead go through `OsString`. We return `ENOTCAPABLE` at the end if there are NULs, or if UTF-8 conversion fails on Windows. This also changes the bounds on some of the methods from `AsRef<str>` to `AsRef<[u8]>`. This shouldn't break any existing code, but allows more flexibility when providing arguments. Depending on the outcome of https://github.com/WebAssembly/WASI/issues/8 we may eventually want to require these bytes be UTF-8, so we might want to revisit this later. Finally, this fixes a tiny bug that could arise if we had exactly the maximum number of file descriptors when populating the preopens.
This is a separate commit, since it changes the interface that downstream clients have to use, and therefore requires a different commit of `wasmtime` for testing. That `wasmtime` commit is currently on my private fork, so this will need to be amended before merging. Now that failures are deferred until `WasiCtxBuilder::build()`, we don't need to have `Result` types on the other methods any longer. Additionally, using `IntoIterator` rather than `Iterator` as the trait bound for these methods is slightly more general, and saves the client some typing.
kubkon
force-pushed
the
acf/ctxbuilder
branch
from
663546a to
c1e1124
Compare
Yeah, that's a real hassle, isn't it? For that very reason, I'll be trying my best to migrate |
|
Sorry I wasn't on the right time zone to offer more feedback here; thank you for merging and good catch with BSD! |
Note that since this involves changes to the interface that downstream clients have to use, it requires a different commit of
wasmtimefor testing. Thatwasmtimecommit is currently on my private fork, so I'm opening this as a draft, as it will need to be amended before merging (edit: see bytecodealliance/wasmtime#490).Text from the commit messages follows:
This changes the fields on the builder to types that let the various
.arg(),.env(), etc methodsinfallible, so we don't have to worry about handling any errors till we actually build. This reduces
line noise when using a builder in a downstream application.
Deferring the processing of the builder fields also has the advantage of eliminating the opening and
closing of
/dev/nullfor the default stdio file descriptors unless they're actually used by theresulting
WasiCtx.Now that failures are deferred until
WasiCtxBuilder::build(), we don't need to haveResulttypeson the other methods any longer.
Additionally, using
IntoIteratorrather thanIteratoras the trait bound for these methods isslightly more general, and saves the client some typing.
Unicode errors when inheriting arguments and environment variables no longer cause a panic, but
instead go through
OsString. We returnENOTCAPABLEat the end if there are NULs, or if UTF-8conversion fails on Windows.
This also changes the bounds on some of the methods from
AsRef<str>toAsRef<[u8]>. Thisshouldn't break any existing code, but allows more flexibility when providing arguments. Depending
on the outcome of WebAssembly/WASI#8 we may eventually want to require
these bytes be UTF-8, so we might want to revisit this later (edit: we now validate UTF-8 during
build()).Finally, this fixes a tiny bug that could arise if we had exactly the maximum number of file
descriptors when populating the preopens, and a bug in the Linux implementation of
isatty().