Re: [PATCH v3 2/2] rust: add module to convert between success/-errno and io::Result

[Zhao Liu <zhao1.liu@intel.com>]

Hi Paolo,

> It is a common convention in QEMU to return a positive value in case of
> success, and a negated errno value in case of error.  Unfortunately,
> using errno portably in Rust is a bit complicated; on Unix the errno
> values are supported natively by io::Error, but on Windows they are not;
> so, use the libc crate.

I'm a bit confused. The doc of error.h just said the negative value for
failure:

• integer-valued functions return non-negative / negative.

Why do we need to using libc's -errno for Windows as well?

Converting `io::Error::last_os_error().raw_os_error().unwrap()` to a
negative value seems compatible with Windows, except it returns Windows
error codes.

> This is a set of utility functions that are used by both chardev and
> block layer bindings.

...

> +// On Unix, from_raw_os_error takes an errno value and OS errors
> +// are printed using strerror.  On Windows however it takes a
> +// GetLastError() value; therefore we need to convert errno values
> +// into io::Error by hand.  This is the same mapping that the
> +// standard library uses to retrieve the kind of OS errors
> +// (`std::sys::pal::unix::decode_error_kind`).
> +impl From<Errno> for ErrorKind {
> +    fn from(value: Errno) -> ErrorKind {

What about `use ErrorKind::*;` to oimt the following "ErrorKind::"
prefix?

> +        let Errno(errno) = value;
> +        match i32::from(errno) {

Maybe `match i32::from(errno.0)` ?

> +            libc::EPERM | libc::EACCES => ErrorKind::PermissionDenied,
> +            libc::ENOENT => ErrorKind::NotFound,
> +            libc::EINTR => ErrorKind::Interrupted,
> +            x if x == libc::EAGAIN || x == libc::EWOULDBLOCK => ErrorKind::WouldBlock,
> +            libc::ENOMEM => ErrorKind::OutOfMemory,
> +            libc::EEXIST => ErrorKind::AlreadyExists,
> +            libc::EINVAL => ErrorKind::InvalidInput,
> +            libc::EPIPE => ErrorKind::BrokenPipe,
> +            libc::EADDRINUSE => ErrorKind::AddrInUse,
> +            libc::EADDRNOTAVAIL => ErrorKind::AddrNotAvailable,
> +            libc::ECONNABORTED => ErrorKind::ConnectionAborted,
> +            libc::ECONNREFUSED => ErrorKind::ConnectionRefused,
> +            libc::ECONNRESET => ErrorKind::ConnectionReset,
> +            libc::ENOTCONN => ErrorKind::NotConnected,
> +            libc::ENOTSUP => ErrorKind::Unsupported,
> +            libc::ETIMEDOUT => ErrorKind::TimedOut,
> +            _ => ErrorKind::Other,

Are these errno cases specifically selected? It seems to have fewer than
`decode_error_kind` lists. Why not support all the cases `decode_error_kind`
mentions? Or do we need to try to cover as many errno cases as possible
from rust/kernel/error.rs?

> +        }
> +    }
> +}
> +
> +// This is used on Windows for all io::Errors, but also on Unix if the
> +// io::Error does not have a raw OS error.  This is the reversed
> +// mapping of the above.

Maybe:

This is the "almost" reversed (except the default case) mapping

?

> +impl From<io::ErrorKind> for Errno {
> +    fn from(value: io::ErrorKind) -> Errno {

`use ErrorKind::*;` could save some words, too.

> +        let errno = match value {
> +            // can be both EPERM or EACCES :( pick one
> +            ErrorKind::PermissionDenied => libc::EPERM,
> +            ErrorKind::NotFound => libc::ENOENT,
> +            ErrorKind::Interrupted => libc::EINTR,
> +            ErrorKind::WouldBlock => libc::EAGAIN,
> +            ErrorKind::OutOfMemory => libc::ENOMEM,
> +            ErrorKind::AlreadyExists => libc::EEXIST,
> +            ErrorKind::InvalidInput => libc::EINVAL,
> +            ErrorKind::BrokenPipe => libc::EPIPE,
> +            ErrorKind::AddrInUse => libc::EADDRINUSE,
> +            ErrorKind::AddrNotAvailable => libc::EADDRNOTAVAIL,
> +            ErrorKind::ConnectionAborted => libc::ECONNABORTED,
> +            ErrorKind::ConnectionRefused => libc::ECONNREFUSED,
> +            ErrorKind::ConnectionReset => libc::ECONNRESET,
> +            ErrorKind::NotConnected => libc::ENOTCONN,
> +            ErrorKind::Unsupported => libc::ENOTSUP,
> +            ErrorKind::TimedOut => libc::ETIMEDOUT,
> +            _ => libc::EIO,
> +        };
> +        Errno(errno as u16)
> +    }
> +}
> +
> +impl From<Errno> for io::Error {
> +    #[cfg(unix)]
> +    fn from(value: Errno) -> io::Error {
> +        let Errno(errno) = value;
> +        io::Error::from_raw_os_error(errno.into())

Maybe `io::Error::from_raw_os_error(value.0.into())`?

> +    }
> +
> +    #[cfg(windows)]
> +    fn from(value: Errno) -> io::Error {
> +        let error_kind: ErrorKind = value.into();
> +        error_kind.into()

Even this works:

     fn from(value: Errno) -> io::Error {
-        let error_kind: ErrorKind = value.into();
-        error_kind.into()
+        value.into()

However, it's less readability, so I still prefer your current codes.
:-)

Thanks,
Zhao