Stream: git-wasmtime

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

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

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

This PR builds on #1243.

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

ToDo:

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

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

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

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

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

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

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

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

This PR builds on #1243.

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

ToDo:

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

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

This PR builds on #1243.

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

ToDo:

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

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

This PR builds on #1243.

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

ToDo:

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

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

This PR builds on #1243.

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

ToDo:

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

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:

Last updated: Dec 23 2024 at 12:05 UTC