Stream: git-wasmtime

Topic: wasmtime / PR #2140 wasi-common: refactor error types

Wasmtime GitHub notifications bot (Aug 18 2020 at 17:57):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 18 2020 at 17:58):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 18 2020 at 18:03):

pchickey requested kubkon for a review on PR #2140.

Wasmtime GitHub notifications bot (Aug 18 2020 at 18:31):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 18 2020 at 18:31):

pchickey requested kubkon and sunfishcode for a review on PR #2140.

Wasmtime GitHub notifications bot (Aug 18 2020 at 22:03):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 18 2020 at 23:06):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 18 2020 at 23:12):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 18 2020 at 23:22):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:25):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, std::io::Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:29):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:31):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:37):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 19:01):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 19:03):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 21:29):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 19 2020 at 22:10):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 24 2020 at 18:02):

pchickey requested sunfishcode and iximeow for a review on PR #2140.

Wasmtime GitHub notifications bot (Aug 25 2020 at 22:23):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Aug 27 2020 at 00:26):

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 of impl From<OtherErrorType> for Errno impls that took care of conversions.

This PR replaces that with a hand-written Error type that contains both the Errno variants the crate actually use, and variants for rich error types like GuestError, Utf8Error, and others. It uses thiserror::Error to derive a nice Error impl. Wiggle's error mapping feature takes care of converting this rich Error into an Errno.

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 wasi Errno.

• 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 an Error::UnexpectedIo(#[source] std::io::Error).
  We convert all of these unexpected into Errno::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 as Errno::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.
-->

Wasmtime GitHub notifications bot (Sep 01 2020 at 18:10):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Sep 01 2020 at 18:10):

iximeow submitted PR Review.

Wasmtime GitHub notifications bot (Sep 01 2020 at 18:10):

iximeow created PR Review Comment:

ERROR_BUFFER_OVERFLOW is .. a fascinatingly-named error.

Wasmtime GitHub notifications bot (Sep 01 2020 at 18:10):

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.

Wasmtime GitHub notifications bot (Sep 01 2020 at 19:57):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Sep 01 2020 at 19:57):

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.

Wasmtime GitHub notifications bot (Sep 01 2020 at 19:58):

pchickey edited PR Review Comment.

Wasmtime GitHub notifications bot (Sep 01 2020 at 20:01):

pchickey merged PR #2140.

Last updated: Nov 22 2024 at 16:03 UTC