𝟎 only log non-zero exit status codes #128
Merged
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
cratelyn
force-pushed
the
katie/zero-exit-status-code
branch
from
549b027 to
c4f7d0a
Compare
aturon
reviewed
Feb 11, 2022
lib/src/execute.rs
Outdated
| event!(Level::ERROR, "WebAssembly trapped: {}", trap); | ||
| // Be sure that we only log non-zero status codes. | ||
| if trap.i32_exit_status() != Some(0) { | ||
| event!(Level::ERROR, "WebAssembly huhu: {}", trap); |
There was a problem hiding this comment.
This LGTM except that I don't know what "huhu" is, was it intended?
There was a problem hiding this comment.
not intended, thank you for catching this! i've force pushed a fresh commit (and commit message) that doesn't include my silly testing string 😅
cratelyn
force-pushed
the
katie/zero-exit-status-code
branch
2 times, most recently
from
34a312b to
bf33880
Compare
|
sigh. i have a sneaking suspicion that i don't see this failure when running |
if a program invokes e.g., `std::process:exit(0)`, this should not cause confusing diagnostic messages as reported in #127: ``` Jan 1 00:00:00.123 ERROR viceroy_lib::execute: WebAssembly trapped: Exited with i32 exit status 0 wasm backtrace: 0: 0x280ea - <unknown>!__wasi_proc_exit 1: 0x280f6 - <unknown>!_Exit ``` this is unfortunately difficult to test, because the `Result<(), ExecutionError>` of `ExecuteCtx::run_guest` is spawned in a task that we do not join on later. (note: that is intentional) the alternative of a test that checks for a particular log message felt brittle, and this is a relatively straightfoward change.
cratelyn
force-pushed
the
katie/zero-exit-status-code
branch
from
bf33880 to
5996ed9
Compare
aturon
approved these changes
Feb 11, 2022
Merged
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.
if a program invokes e.g.,
std::process:exit(0), this should not causeconfusing messages as reported in #127:
apologies for the lack of test coverage here.
this is unfortunately difficult to test, because the
Result<(), ExecutionError>ofExecuteCtx::run_guestis spawned in atask that we do not join on later. (note: that is intentional)
the alternative of a test that checks for a particular log message felt
brittle, and this is a relatively straightfoward change.