Stream: git-wasmtime

Topic: wasmtime / PR #1242 [wasi-common]: yanix now returns io::...

Wasmtime GitHub notifications bot (Mar 06 2020 at 13:06):

kubkon edited PR #1242 from yanix-io-error to master:

This PR may seem somewhat controversial at first, but hear me
out first. Currently, Yanix would return a custom error that's a
wrapper around three other error types returned by various entities
inside Rust's libstd. In particular, Yanix's error type would wrap
io::Error, num::TryFromIntError and ffi::NulError. It turns
out that there is a natural conversion between the first and the last
and provided by the standard library, i.e., From<ffi::NulError> for io::Error
is provided. So at the surface it may seem that only the first two
wrapped error types are worth keeping.

Digging a little bit deeper into libstd, num::TryFromIntError
is essentially speaking only a marker that the integral conversion
went wrong. The struct implementing this error stores a unit type,
and nothing more. It therefore seems like a waste to wrap this
particular error when we could unify everything under io::Error.
And so, whenever we perform an int conversion, I suggest we simply
remap the error to io::Error::from_raw_os_error(libc::EOVERFLOW)
since this carries a comparable amount of information.

As a result of completely discarding yanix::Error custom error type,
we are invariably simplifying yanix itself, but also allowing
wasi-common to simplify in several places as well.

I'm curious to see what you guys reckon!

Wasmtime GitHub notifications bot (Mar 06 2020 at 13:09):

kubkon updated PR #1242 from yanix-io-error to master:

This PR may seem somewhat controversial at first, but hear me
out first. Currently, Yanix would return a custom error that's a
wrapper around three other error types returned by various entities
inside Rust's libstd. In particular, Yanix's error type would wrap
io::Error, num::TryFromIntError and ffi::NulError. It turns
out that there is a natural conversion between the first and the last
and provided by the standard library, i.e., From<ffi::NulError> for io::Error
is provided. So at the surface it may seem that only the first two
wrapped error types are worth keeping.

Digging a little bit deeper into libstd, num::TryFromIntError
is essentially speaking only a marker that the integral conversion
went wrong. The struct implementing this error stores a unit type,
and nothing more. It therefore seems like a waste to wrap this
particular error when we could unify everything under io::Error.
And so, whenever we perform an int conversion, I suggest we simply
remap the error to io::Error::from_raw_os_error(libc::EOVERFLOW)
since this carries a comparable amount of information.

As a result of completely discarding yanix::Error custom error type,
we are invariably simplifying yanix itself, but also allowing
wasi-common to simplify in several places as well.

I'm curious to see what you guys reckon!

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:02):

kubkon edited PR #1242 from yanix-io-error to master:

This PR may seem somewhat controversial at first, but hear me out first. Currently, Yanix would return a custom error that's a wrapper around three other error types returned by various entities inside Rust's libstd. In particular, Yanix's error type would wrap io::Error, num::TryFromIntError and ffi::NulError. It turns out that there is a natural conversion between the first and the last and provided by the standard library, i.e., From<ffi::NulError> for io::Error is provided. So at the surface it may seem that only the first two wrapped error types are worth keeping.

Digging a little bit deeper into libstd, num::TryFromIntError is essentially speaking only a marker that the integral conversion went wrong. The struct implementing this error stores a unit type, and nothing more. It therefore seems like a waste to wrap this particular error when we could unify everything under io::Error. And so, whenever we perform an int conversion, I suggest we simply remap the error to io::Error::from_raw_os_error(libc::EOVERFLOW) since this carries a comparable amount of information.

As a result of completely discarding yanix::Error custom error type, we are invariably simplifying yanix itself, but also allowing wasi-common to simplify in several places as well.

I'm curious to see what you guys reckon!

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode created PR Review Comment:

We can just unwrap() this, because readlinkat returns an ssize_t which is either -1 (handled above) or non-negative and will fit in a size_t/usize, which is what we're converting it to here.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode created PR Review Comment:

FIONREAD returns a non-negative int if it doesn't fail, or it'll fit in a u32 if it does. Or, if we want to be super cautious and avoid assuming int is 32-bit, we could widen fionread's return type here, since the one place that calls it wants a u64 anyway. But either way, we can unwrap() the conversion from int here.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode created PR Review Comment:

This can overflow, and it's an error if it does, but: the cookie is an opaque value, and applications aren't supposed to do arithmetic on them or pick their own values or have any awareness of the numeric range of the values. They're just supposed to pass back in the values that we give them. And any value we give them will be convertable back to long, because that's the type the OS gives them to us in. So, this isn't an EOVERFLOW, it's an EINVAL.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode created PR Review Comment:

lseek returns an off_t, which we can assume is a non-negative i64 if it doesn't fail. So we can unwrap() this conversion.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode created PR Review Comment:

This conversion can fail, because it's converting into int. But in that case, the user is providing a dubiously large hint. I don't know specifically how F_RDADVISE works, but offhand, a 2+ GiB advisory read async seems unlikely to help with any kind of performance, so my suggestion here would be to just have this function do nothing if the size overflows here.

Wasmtime GitHub notifications bot (Mar 06 2020 at 14:34):

sunfishcode created PR Review Comment:

When poll doesn't fail, its return value is a non-negative int, which will always be convertable to usize, so we can unwrap() here.

Wasmtime GitHub notifications bot (Mar 06 2020 at 15:37):

kubkon created PR Review Comment:

Right. I won't widen the return type to u64 at this stage, but I'll leave a note (essentially a copy-paste of your excellent explanation) for future consideration should we want to re-visit this. Does it makes sense?

Wasmtime GitHub notifications bot (Mar 06 2020 at 15:37):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 15:48):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 15:48):

kubkon created PR Review Comment:

SGTM! If I may expand on this a little bit, how about we log::warn if that's the case before returning early? This should provide some feedback to the user that they may have something wrongly configured, etc.

Wasmtime GitHub notifications bot (Mar 06 2020 at 15:52):

kubkon updated PR #1242 from yanix-io-error to master:

This PR may seem somewhat controversial at first, but hear me out first. Currently, Yanix would return a custom error that's a wrapper around three other error types returned by various entities inside Rust's libstd. In particular, Yanix's error type would wrap io::Error, num::TryFromIntError and ffi::NulError. It turns out that there is a natural conversion between the first and the last and provided by the standard library, i.e., From<ffi::NulError> for io::Error is provided. So at the surface it may seem that only the first two wrapped error types are worth keeping.

Digging a little bit deeper into libstd, num::TryFromIntError is essentially speaking only a marker that the integral conversion went wrong. The struct implementing this error stores a unit type, and nothing more. It therefore seems like a waste to wrap this particular error when we could unify everything under io::Error. And so, whenever we perform an int conversion, I suggest we simply remap the error to io::Error::from_raw_os_error(libc::EOVERFLOW) since this carries a comparable amount of information.

As a result of completely discarding yanix::Error custom error type, we are invariably simplifying yanix itself, but also allowing wasi-common to simplify in several places as well.

I'm curious to see what you guys reckon!

Wasmtime GitHub notifications bot (Mar 06 2020 at 16:00):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 16:00):

sunfishcode created PR Review Comment:

Good idea!

Wasmtime GitHub notifications bot (Mar 06 2020 at 16:11):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 16:11):

sunfishcode created PR Review Comment:

Yes. And actually, there's a subtlety I didn't mention. If you call FIONREAD on a file, it can overflow the int, and at least Linux doesn't check the overflow, but we have code to handle that case in the code that calls this function, and that's what lets us assume that it won't overflow. Fortunately, an unwrap() means that if any OS ever does give us a negative number that we don't expect, it won't pass by silently.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:24):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:24):

alexcrichton created PR Review Comment:

FWIW I think ideally we would use nread.try_into()? here. I agree with the comment here but "unwrap" is pretty scary when reading this sort of code and feels like something that would best be entirely blanket avoided. If the ?-conversion works it's also even more ergonomic than nread.try_into().unwrap()!

Basically I like that there's a comment here to explain why the unwrap is ok, and I don't disagree with it, but I think it would be better idiomatically to use ? if we can to propagate errors to consistently "never use unwrap" anywhere in wasi-common. (in theory at least). If the error handling is more verbose than the literal 1-character ? symbol though then this is fine.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:25):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:25):

alexcrichton created PR Review Comment:

Wanna go ahead and fix this for other platforms and remove the : i64 here?

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:27):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:27):

alexcrichton created PR Review Comment:

I've seen this pattern in a few places at this point, and the .unwrap() here makes me a little uncomfortable. Could this perhaps be replaced with either a function that returns the last raw os error, or a function that extracts the raw os error and returns some other error if it can't be extracted? For example something like:

fn last_raw_os_error() -> i32 {
    Error::last_os_error().raw_os_error().unwrap_or(-1)
}

or something like:

fn raw_os_error(err: &io::Error) -> Result<i32> {
    err.raw_os_error().ok_or_else(Errno::EIO)
}

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:37):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 19:37):

alexcrichton created PR Review Comment:

@sunfishcode has some counterpoints to this thinking on Zulip, so I don't think that we should remove the .unwrap() for now. Let's leave it here and handle the bug report if it ever comes in!

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:05):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:05):

sunfishcode created PR Review Comment:

The docs for raw_os_error guarantee that it'll return Some on an error returned from last_os_error, so the unwrap is guaranteed to succeed.

It doesn't matter so much in isatty here, but across wasi-common we often have subtle reasoning about specific error codes, so introducing new conditions under which errors like EIO or even -1 could materialize makes that tricky.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:22):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:22):

kubkon created PR Review Comment:

FWIW, both of you guys make really good points. So in order to strike a middle ground, I've made sure to leave comments at every occurrence of unwrap.

OK, so ultimately let's stick with unwraps for now, until we get some bug reports. :+1:

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:23):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:23):

kubkon created PR Review Comment:

Ah, you're probably referring to #1231. Good call, lemme do just that!

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:28):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:28):

kubkon created PR Review Comment:

As @sunfishcode pointed out, according to the docs, this cannot ever fail as we only ever call unwrap after io::Error was populated with a raw OS error, either with io::Error::last_os_error() or io::Error::from_raw_os_error(). I do agree with you @alexcrichton that this is pretty subtle in the sense that we're relying on this prior knowledge that we've actually done just that. If it wasn't the case, we'd in a lot of trouble. Then again, since yanix, and in general, the syscall impl code in wasi-common _only_ (at least for now!) deals with raw OS errno values, this is safe. And since we're constantly evolving WASI, having panics around those calls might be a good thing to catch bugs that otherwise would be silently swallowed as @sunfishcode pointed out.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:28):

kubkon edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:48):

kubkon updated PR #1242 from yanix-io-error to master:

This PR may seem somewhat controversial at first, but hear me out first. Currently, Yanix would return a custom error that's a wrapper around three other error types returned by various entities inside Rust's libstd. In particular, Yanix's error type would wrap io::Error, num::TryFromIntError and ffi::NulError. It turns out that there is a natural conversion between the first and the last and provided by the standard library, i.e., From<ffi::NulError> for io::Error is provided. So at the surface it may seem that only the first two wrapped error types are worth keeping.

Digging a little bit deeper into libstd, num::TryFromIntError is essentially speaking only a marker that the integral conversion went wrong. The struct implementing this error stores a unit type, and nothing more. It therefore seems like a waste to wrap this particular error when we could unify everything under io::Error. And so, whenever we perform an int conversion, I suggest we simply remap the error to io::Error::from_raw_os_error(libc::EOVERFLOW) since this carries a comparable amount of information.

As a result of completely discarding yanix::Error custom error type, we are invariably simplifying yanix itself, but also allowing wasi-common to simplify in several places as well.

I'm curious to see what you guys reckon!

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:49):

kubkon submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 20:49):

kubkon created PR Review Comment:

Done in ed98a80.

Wasmtime GitHub notifications bot (Mar 06 2020 at 21:24):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 21:59):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 06 2020 at 22:20):

sunfishcode merged PR #1242.

Last updated: Dec 23 2024 at 12:05 UTC