fix(wasi) Fix the logic behind inherited/captured stdin, stdout and stderr
#2059
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd `stderr`. First, let's no longer derive from `Default` for `wasi_config_t`. By default, we want to inherit `stdin`, `stdout` and `stderr`. The default for `bool` is `false`; we want `true` here. Second, let's update `wasi_config_new` to correctly set `inherit_*` fields to `true`. Third, lets' create `wasi_config_capture_*` functions. By default, `std*` are inherited, so we need functions to capture them. That's the new feature this patch introduces. The `wasi_config_inherit_*` functions are kept for the sake of backward compatibility. Ideally, we would want an API like `wasi_config_capture_*(capture: bool)`, but it would duplicate the API somehow. Fourth, let's fix `wasi_env_new`. We want to capture `stdout` and `stderr` if and only if the `inherit_*` fields are set to `false`. There was bug here. That's why everything was working correctly by the way: `bool::default()` is `false`, and we have this inverted condition here, so everything was working as expected because of a double error. The only bug was that it wasn't possible to capture `std*` before.
Hywan
added
bug
|
bors try |
MarkMcCaskey
approved these changes
Jan 26, 2021
lib/c-api/src/wasm_c_api/wasi/mod.rs
Outdated
Comment on lines
155
to
160
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn wasi_config_capture_stdin(config: &mut wasi_config_t) { | ||
| config.inherit_stdin = false; | ||
| } | ||
|
|
There was a problem hiding this comment.
What does it mean to capture stdin here?
I don't think we have any APIs to make use of this, perhaps keep it commented out so we don't export this symbol until we have a use for it.
There was a problem hiding this comment.
Let's keep wasi_config_capture_stdin apart for the moment :-).
|
bors r+ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This patch is both a fix and a feature!
First, let's no longer derive from
Defaultforwasi_config_t. Bydefault, we want to inherit
stdin,stdoutandstderr. Thedefault for
boolisfalse; we wanttruehere.Second, let's update
wasi_config_newto correctly setinherit_*fields to
true.Third, lets' create
wasi_config_capture_*functions. By default,std*are inherited, so we need functions to capture them. That's thenew feature this patch introduces. The
wasi_config_inherit_*functions are kept for the sake of backward compatibility. Ideally, we
would want an API like
wasi_config_capture_*(capture: bool), but itwould duplicate the API somehow.
Fourth, let's fix
wasi_env_new. We want to capturestdoutandstderrif and only if theinherit_*fields are set tofalse. There was bug here. That's why everything was workingcorrectly by the way:
bool::default()isfalse, and we have thisinverted condition here, so everything was working as expected because
of a double error. The only bug was that it wasn't possible to capture
std*before.Review