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) reusestd::io::Error
to generate and wrap raw *nix errno value- update
wasi-common
to use new way of handling raw OS error inyanix
; i.e., via re-use ofstd::io::Error
instead of a customErrno
enum
kubkon requested alexcrichton, and sunfishcode for a review on PR #1226.
kubkon requested alexcrichton, and sunfishcode for a review on PR #1226.
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) reusestd::io::Error
to generate and wrap raw *nix errno value- update
wasi-common
to use new way of handling raw OS error inyanix
; i.e., via re-use ofstd::io::Error
instead of a customErrno
enum
sunfishcode submitted PR Review.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
raw_os_error
will always returnSome
when the error comes fromlast_os_error
, so you can justunwrap()
it here, and same for the other places in the code where this comes up.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Or actually, given what this code is doing, this should fall-through to returning
err
as usual?
alexcrichton submitted PR Review.
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 theErr
payload of the previous call tofstatat
?
kubkon submitted PR Review.
kubkon created PR Review Comment:
So the reason we make another call to
errno
here is sincefstatat
can fail as well, so we've got a dilemma whether 1. we should throw the originalerr
(and perhaps that's the right thing to do), and 2. report the latesterrno
instead (the one generated byfstatat
). Thoughts?
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.
kubkon submitted PR Review.
alexcrichton submitted PR Review.
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.
kubkon submitted PR Review.
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. :-)
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) reusestd::io::Error
to generate and wrap raw *nix errno value- update
wasi-common
to use new way of handling raw OS error inyanix
; i.e., via re-use ofstd::io::Error
instead of a customErrno
enum
kubkon submitted PR Review.
kubkon created PR Review Comment:
Done in 686fd9d. Hopefully I got all of them!
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
.
sunfishcode submitted PR Review.
kubkon created PR Review Comment:
Good point!
kubkon submitted PR Review.
kubkon merged PR #1226.
Last updated: Dec 23 2024 at 12:05 UTC