Open jethrogb opened 7 years ago
There is From<Errno> for io::Error
. Does this not work for your use case?
I'm sure I looked for that but couldn't find it in the documentation! I now realize that it wouldn't be in the documentation at all (https://github.com/rust-lang/rust/issues/42440). In any case, this is not sufficient because errno doesn't store the original error code, so unknown codes get clobberred into 0
.
Okay, I think I understand the issue here. You want the i32
of the Errno
back. Could you provide a code example that demonstrates your issue?
Right now there is impl From<Errno> for io::Error
that should allow you to do example what you want using .into()
. Maybe that wasn't discoverable for you or is simply less ergonomic than you like?
There are two separate issues.
i32
to Errno
to io::Error
back to i32
is lossy:
#[test]
fn errno_conversion() {
assert_eq!(std::io::Error::from(nix::Errno::from_i32(1234)).raw_os_error(), Some(1234));
}
thread 'errno_conversion' panicked at 'assertion failed: `(left == right)` (left: `Some(0)`, right: `Some(1234)`)', src/lib.rs:5
Regarding the first point, I definitely agree. I think we would be better served to move all error handing into an error.rs
and then provide module-level documentation explaining the various tools we provide for people to interface with the Error
class.
As for the second point, that behavior is because 1234
isn't a valid errno
. I actually think that function should be changed to return a Result
. I don't see why nix
should be supportive of unknown errno
s since they're so constant.
@jethrogb Would you be willing to add some docs to the errno module explaining this conversion? As to your second point, I don't think we need to support arbitrary errno
s, but maybe you have a use case we hadn't considered?
I actually think that function should be changed to return a Result.
By “that function” do you mean Errno::from_i32
? I don't think that's a good idea, this function is used to convert error codes returned when interfacing with C functions. By the time you're calling Errno::from_i32
you know you have a valid errno value from somewhere else.
Errno's do occasionally change https://github.com/torvalds/linux/blame/master/include/uapi/asm-generic/errno.h https://github.com/opensource-apple/xnu/blame/10.12/bsd/sys/errno.h
Newly introduced error codes might not make much sense to old binaries interpreting them, but at least they can mean something to the user (either in raw form or through strerror by dynamically linking to libc). It seems like a waste to throw away this valuable error information.
@jethrogb I'd be happy to consider a more concrete solution here along if it also showed specific use cases the current system doesn't allow or are ergonomic. Please file a PR if you have an idea for what you want and think is reasonable to implement and we can iterate there I think.
Here's a related issue that doesn't work right now
fn io_func() -> io::Result<()> {
std::fs::some_function()?;
nix::some_other_function()?;
}
@kahing That shouldn't work. Not all error types are compatible between libraries, nor are they designed to be compatible. There is no appropriate mapping that works in all use cases from nix::Error
to io::Error
, which is why one is not provided.
In your code example, you should invent a new error type that provides a From<io::Error>
and a From<nix::Error>
and use that as the return type to this function.
@jethrogb Are you interested in filing a PR and driving a resolution for this issue?
I understand that they are not compatible but since rust io and nix are often used together, it would be nice if they were compatible out of the box. (yes, I do have my own error type now).
I think it makes a lot of sense to provide From<nix::Error>
for io::Error
. If the original error is an Errno
, it's an obvious conversion, otherwise, it can be a io::ErrorKind::Other
.
I'll look into doing a PR at some point
@jethrogb we removed it in #614, we won't be adding it back in, so need to work on a PR for this. The rust-y way to handle this is to create your own error type and map from all other errors into that error type for export.
@jethrogb If you want to file a PR for the other things we've discussed in this issue, that would definitely be considered.
I would love to see From<nix::Error>
for io::Error
in nix.
Reasoning: People are writing their own conversions. @paulpr0 is doing it in the reference above, and I will be doing it too. Having a good impl in nix would reduce friction for users who want to manage errors in this way.
Would the existence of From<nix::Error>
for io::Error
break existing code or cause other problems?
I'm using ioctl_*!
macros, and my conversion to std::io::Error
is std::io::Error::from(err.as_errno().unwrap())
.
This is safe with the current implementation, because it internally uses nix::Error::result
, that always creates Error::Sys
variant for Error
:
https://github.com/nix-rust/nix/blob/a4a465d25567f163f9552b977eb4d17435251d41/src/errno.rs#L77-L86
But, in fact, there's absolutely no need for that nix::Error
type to take place at for ioctl
call, and bare Result<c_int, Errno>
should be used instead.
The error conversion would then simply be done via ?
as expected.
What I'm saying is that current API for ioctl
is not designed very wisely, and can be simplified for the better. Maybe the same applies for the rest of the errors nix
produces? It might be that nix::Error
is actually usable only in a handful of cases, and in that sense should not be used everywhere in the nix
(as an error type by default) as it is used currently.
I propose reviewing the code to find all places where using nix::Error
is actually required, split it into multiple types that represent possible error conditions at a particular code more accurately (i.e. some fns can't ever return UnsupportedOperation
variant, and some can't ever return InvalidPath
) and rewrite the code to return the most specific (i.e. the most accurately representing actual real failure scenarios) error type.
We can then reconstruct the nix::Error
type from every possible error condition to provide a one-size-fits-all type, though we might also not want to provide that because there are a lot of alternatives for the users (like using Box<dyn std::error::Error>
or using failure
crate). And, again, we'll only need it for the users, cause our API would be covered with more specific error types.
I think this now served by std::io::Error::from_raw_os_error(errno as i32)
, right? I mean, both match against the same libc::E*
constants when round tripping to/from i32
.
@ExceptionallyHandsome https://github.com/nix-rust/nix/issues/613#issuecomment-306118919
this is not sufficient because errno doesn't store the original error code, so unknown codes get clobberred into
0
.
There will likely be an io::ErrorKind::NotSupported
added in Rust 1.52 (rust-lang/rust#78880), I think that be a good mapping for nix::Error::UnsupportedOperation
if From<nix::Error> for io::Error
were ever to be added again.
Also, I'd like to make a case for why that impl should be added again: nix::Error
is basically a subset of io::Error
. While io::Error
can hold an arbitrary user error of any ErrorKind or an errno, nix::Error
can hold a user error of kind UnsupportedOperation, InvalidPath, InvalidUtf8
or an errno. IMO there's a very clear mapping between the latter and the former, and I don't think it makes sense to have to make a custom error type just because you have code that does both stdlib io and unix-specific io. Obviously it's a good idea to have nix::Error
as its own separate type, since it vastly reduces the set of errors that can exist and it makes it much easier to handle, but if you do need to "widen" the error type, I think converting to io::Error
makes more sense than having an enum Error { Nix(nix::Error), Io(io::Error) }
There are situation where you want to have the raw error value of an error, e.g. for passing back into C code. Also, there is already a perfectly good type for holding errno in Rust,
std::io::Error
. It does support getting the raw error value. I suggest deprecatingErrno
in favor ofstd::io::Error
or at least adding the necessary conversions.