wasmtime / PR #1226 wasi-common error cleanup: part 1, yanix · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1226 wasi-common error cleanup: part 1, yanix

Wasmtime GitHub notifications bot (Mar 04 2020 at 10:56):

kubkon opened PR #1226 from yanix-error-cleanup to master:

This is the first PR in a series of PRs aiming at streamlining and future-proofing wasi-common's error handling. Changes included in this part of the series:

remove custom yanix::Errno and instead (as was previously suggested) reuse std::io::Error to generate and wrap raw *nix errno value

update wasi-common to use new way of handling raw OS error in yanix; i.e., via re-use of std::io::Error instead of a custom Errno enum

Wasmtime GitHub notifications bot (Mar 04 2020 at 10:56):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1226.

Wasmtime GitHub notifications bot (Mar 04 2020 at 10:56):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1226.

Wasmtime GitHub notifications bot (Mar 04 2020 at 10:58):

kubkon updated PR #1226 from yanix-error-cleanup to master:

This is the first PR in a series of PRs aiming at streamlining and future-proofing wasi-common's error handling. Changes included in this part of the series:

remove custom yanix::Errno and instead (as was previously suggested) reuse std::io::Error to generate and wrap raw *nix errno value

update wasi-common to use new way of handling raw OS error in yanix; i.e., via re-use of std::io::Error instead of a custom Errno enum

Wasmtime GitHub notifications bot (Mar 04 2020 at 14:55):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 14:55):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 14:55):

sunfishcode created PR Review Comment:

raw_os_error will always return Some when the error comes from last_os_error, so you can just unwrap() it here, and same for the other places in the code where this comes up.

Wasmtime GitHub notifications bot (Mar 04 2020 at 15:31):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 15:31):

alexcrichton created PR Review Comment:

Or actually, given what this code is doing, this should fall-through to returning err as usual?

Wasmtime GitHub notifications bot (Mar 04 2020 at 15:31):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 15:31):

alexcrichton created PR Review Comment:

I realize that this was already existing code, but I dont understand why this is calling last_os_error vs just using the Err payload of the previous call to fstatat?

Wasmtime GitHub notifications bot (Mar 04 2020 at 19:02):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 19:02):

kubkon created PR Review Comment:

So the reason we make another call to errno here is since fstatat can fail as well, so we've got a dilemma whether 1. we should throw the original err (and perhaps that's the right thing to do), and 2. report the latest errno instead (the one generated by fstatat). Thoughts?

Wasmtime GitHub notifications bot (Mar 04 2020 at 19:02):

kubkon created PR Review Comment:

Makes sense, I'll make the adjustments. This should clean up the calls since this way we'll effectively get rid of one level of nesting.

Wasmtime GitHub notifications bot (Mar 04 2020 at 19:02):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 19:57):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 19:57):

alexcrichton created PR Review Comment:

In general for polyfill-style operations like this I tend to err on the side of returning the original error whenever possible, largely just optionally logging other errors if they happen.

Wasmtime GitHub notifications bot (Mar 04 2020 at 20:13):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 20:13):

kubkon created PR Review Comment:

SGTM! For anyone "listening in" on this discussion, I'd like to propose that we stick to this convention from now on to be consistent. :-)

Wasmtime GitHub notifications bot (Mar 04 2020 at 20:49):

kubkon updated PR #1226 from yanix-error-cleanup to master:

This is the first PR in a series of PRs aiming at streamlining and future-proofing wasi-common's error handling. Changes included in this part of the series:

remove custom yanix::Errno and instead (as was previously suggested) reuse std::io::Error to generate and wrap raw *nix errno value

update wasi-common to use new way of handling raw OS error in yanix; i.e., via re-use of std::io::Error instead of a custom Errno enum

Wasmtime GitHub notifications bot (Mar 04 2020 at 20:50):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 20:50):

kubkon created PR Review Comment:

Done in 686fd9d. Hopefully I got all of them!

Wasmtime GitHub notifications bot (Mar 04 2020 at 21:01):

sunfishcode created PR Review Comment:

It's a good philosophy in general, though for WASI APIs, the ultimate consideration is, what error code best explains the failure to the WASI user who doesn't know that we're doing multiple syscalls? In this case, I agree that we should try to return the error from unlinkat.

Wasmtime GitHub notifications bot (Mar 04 2020 at 21:01):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 04 2020 at 21:11):

kubkon created PR Review Comment:

Good point!

Wasmtime GitHub notifications bot (Mar 04 2020 at 21:11):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 05 2020 at 09:08):

kubkon merged PR #1226.

Last updated: Nov 22 2024 at 17:03 UTC