pchickey opened PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey requested kubkon for a review on PR #2140.
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey requested kubkon and sunfishcode for a review on PR #2140.
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,std::io::Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey edited PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, and I don't have any firm
guarantees that nobody was depending on the old behavior, but it
appears to me that none of those unexpected errnos were reasonable
to expect from any of the filesystem syscalls wasi-common is making.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey edited PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey requested sunfishcode and iximeow for a review on PR #2140.
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
pchickey updated PR #2140 from pch/wasi_error_handling
to main
:
Prior to this PR, wasi-common used the wiggle-generated
Errno
type throughout, and had a variety ofimpl From<OtherErrorType> for Errno
impls that took care of conversions.This PR replaces that with a hand-written
Error
type that contains both theErrno
variants the crate actually use, and variants for rich error types likeGuestError
,Utf8Error
, and others. It usesthiserror::Error
to derive a niceError
impl. Wiggle's error mapping feature takes care of converting this richError
into anErrno
.This decouples a huge amount of code in wasi-common from the details of the wiggle-generated type, which will be useful as we start supporting multiple snapshots through wiggle (i'm working on that now). It also means we can keep around richer error information for longer, which is better for error reporting. Rather than having ad-hoc log statements sprinkled in various
From
impls, we rely on the error conversion hooks that Wiggle provides to perform logging.This PR also changes, in a subtle way, the way a
std::io::Error
gets translated through to a wasiErrno
.
a certain subset of io::Errors are expected - these we have
a (platform-specific, because windows) method to translate into
one of the wasi errno variants in the Error enum.some io::Errors are unexpected - wasi-common doesnt expect them from
the underlying OS. rather than preserve any fidelity in reporting
those to the user (only the unix impl attempts this), lets collect
those as anError::UnexpectedIo(#[source] std::io::Error)
.
We convert all of these unexpected intoErrno::Io
for returning
to the guest.This is a different behavior from before, where e.g. if the OS returned a
libc::ECONNABORTED
we would surface that to the guest asErrno::Econnaborted
, and I don't have any firm guarantees that nobody was depending on the old behavior, but it appears to me that none of those unexpected errnos were reasonable to expect from any of the filesystem syscalls wasi-common is making. When wasi-common expands in responsibility to include sockets syscalls etc, we can add these behaviors back in explicitly.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
iximeow submitted PR Review.
iximeow submitted PR Review.
iximeow created PR Review Comment:
ERROR_BUFFER_OVERFLOW
is .. a fascinatingly-named error.
iximeow created PR Review Comment:
Err(Error::Inval) | Err(Error::Noent) => {} Err(Error::Notdir) => {
I'm having a hard time reconstructing how this works with symlinks, but I think the comment is describing the specific
readlinkat-returns-Notdir
case.
pchickey submitted PR Review.
pchickey created PR Review Comment:
Me too. This is a transformation of code that existed before (when we had Eq; we no longer do) that I am not 100% convinced is correct, but don't have a sufficient understanding of the problem or test suite to disprove.
pchickey edited PR Review Comment.
pchickey merged PR #2140.
Last updated: Dec 23 2024 at 12:05 UTC