Stream: git-wasmtime

Topic: wasmtime / PR #1253 [wasi-common]: clean up error handling

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 20:27):

kubkon edited PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 20:51):

kubkon updated PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 21:11):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 21:11):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 21:11):

sunfishcode created PR Review Comment:

This is a super minor nit, but it feels awkward to me to return Utf8Error as its own arm of WasiCtxBuilderResult while mapping NulError to io::Error. Also, the NulErrors we get don't arise from doing any actual I/O. What would you think of adding a new WasiCtxBuilderError arm for this?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 21:11):

sunfishcode created PR Review Comment:

Is this used in this patch?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2020 at 21:11):

sunfishcode created PR Review Comment:

These errors are likely to be fairly visible, and unlike POSIX where ENOTDIR is typically returned from simple system calls where you can pretty much guess which thing it was that wasn't a directory that should have been, these are returned from WasiCtxBuilder::build, which does many things. So I suggest adding a path field to this error and making the message more specific, mentioning that this is about the preopens. Something like "Preopen directory path {} is not a directory", so that it's clear what the problem is.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:49):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:49):

kubkon created PR Review Comment:

Ah, excellent catch! A leftover from prior attempts, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:49):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:49):

kubkon created PR Review Comment:

Makes sense! Lemme fix then!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:50):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 10:50):

kubkon created PR Review Comment:

I totally agree! The more info we can pass back to the client at this stage, the better!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2020 at 12:05):

kubkon updated PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 09:32):

kubkon edited PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 09:36):

kubkon updated PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 15:07):

kubkon edited PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 15:22):

kubkon updated PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 15:24):

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 16:35):

kubkon updated PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 19:08):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 20:42):

kubkon updated PR #1253 from error-cleanup to master:

WasiCtxBuilderError is the wasi-common client-facing error type which is exclusively thrown when building a new WasiCtx instance. As such, building such an instance should not require the client to understand different WASI errno values as was assumed until now.

This commit is a first step at streamlining error handling in wasi-common and makes way for the wiggle crate.

When adding the WasiCtxBuilderError, I've had to do two things of notable importance:

  1. I've removed a couple of ok_or calls in WasiCtxBuilder::build and replaced them with unwraps, following the same pattern in different builder methods above. This is fine since we _always_ operate on non-empty Options in WasiCtxBuilder thus unwraping will never fail. On the other hand, this might be a good opportunity to rethink the structure of our builder, and how we good remove the said Options especially since we always populate them with empty containers to begin with. I understand this is to make chaining of builder methods easier which take and return &mut self and the same applies to WasiCtxBuilder::build(&mut self) method, but perhaps it would more cleanly signal the intentions if we simply moved WasiCtxBuilder instance around. Food for thought!
  2. Methods specific to determining rights of passed around std::fs::File objects when populating WasiCtx FdEntry entities now return io::Error directly so that we can reuse them in WasiCtxBuilder methods (returning WasiCtxBuilderError error type), and in syscalls (returning WASI errno).

With WasiCtxBuilderError in place, we now can return a pure WasiError in all syscalls-related functions. This means we can completely remove error::Error type, and now, io::Error and
related are automatically converted to their corresponding WASI errno value encapsulated as WasiError. IMHO this makes tracking errors in syscalls a lot cleaner and simpler.

While here, it also made sense to me to move WasiError to wasi module which will align itself well with the upcoming changes introduced by wiggle. To different standard Result from WASI specific, I've created a helper alias WasiResult also residing in wasi module.

Prereqs:

OK, while technically we only depend on #1243 since we're rebased on it, it'd be best if we landed #1255 before as well. So:

ToDo:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2020 at 21:58):

kubkon merged PR #1253.

Last updated: Nov 22 2024 at 17:03 UTC