Message-ID: <20240916210111.502e7d6d.gary@garyguo.net>
Date: Mon, 16 Sep 2024 21:01:11 +0100
From: Gary Guo <gary@...yguo.net>
To: Yiyang Wu <toolmanp@...p.cc>
Cc: linux-erofs@...ts.ozlabs.org, rust-for-linux@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 03/24] erofs: add Errno in Rust

On Mon, 16 Sep 2024 21:56:13 +0800
Yiyang Wu <toolmanp@...p.cc> wrote:

> Introduce Errno to Rust side code. Note that in current Rust For Linux,
> Errnos are implemented as core::ffi::c_uint unit structs.
> However, EUCLEAN, a.k.a EFSCORRUPTED is missing from error crate.
> 
> Since the errno_base hasn't changed for over 13 years,
> This patch merely serves as a temporary workaround for the missing
> errno in the Rust For Linux.
> 
> Signed-off-by: Yiyang Wu <toolmanp@...p.cc>

As Greg said, please add missing errno that you need to kernel crate
instead.

Also, it seems that you're building abstractions into EROFS directly
without building a generic abstraction. We have been avoiding that. If
there's an abstraction that you need and missing, please add that
abstraction. In fact, there're a bunch of people trying to add FS
support, please coordinate instead of rolling your own.

You also have been referencing `kernel::bindings::` directly in various
places in the patch series. The module is marked as `#[doc(hidden)]`
for a reason -- it's not supposed to referenced directly. It's only
exposed so that macros can reference them. In fact, we have a policy
that direct reference to raw bindings are not allowed from drivers.

There're a few issues with this patch itself that I pointed out below,
although as already said this would require big changes so most points
are probably moot anyway.

Thanks,
Gary

> ---
>  fs/erofs/rust/erofs_sys.rs        |   6 +
>  fs/erofs/rust/erofs_sys/errnos.rs | 191 ++++++++++++++++++++++++++++++
>  2 files changed, 197 insertions(+)
>  create mode 100644 fs/erofs/rust/erofs_sys/errnos.rs
> 
> diff --git a/fs/erofs/rust/erofs_sys.rs b/fs/erofs/rust/erofs_sys.rs
> index 0f1400175fc2..2bd1381da5ab 100644
> --- a/fs/erofs/rust/erofs_sys.rs
> +++ b/fs/erofs/rust/erofs_sys.rs
> @@ -19,4 +19,10 @@
>  pub(crate) type Nid = u64;
>  /// Erofs Super Offset to read the ondisk superblock
>  pub(crate) const EROFS_SUPER_OFFSET: Off = 1024;
> +/// PosixResult as a type alias to kernel::error::Result
> +/// to avoid naming conflicts.
> +pub(crate) type PosixResult<T> = Result<T, Errno>;
> +
> +pub(crate) mod errnos;
>  pub(crate) mod superblock;
> +pub(crate) use errnos::Errno;
> diff --git a/fs/erofs/rust/erofs_sys/errnos.rs b/fs/erofs/rust/erofs_sys/errnos.rs
> new file mode 100644
> index 000000000000..40e5cdbcb353
> --- /dev/null
> +++ b/fs/erofs/rust/erofs_sys/errnos.rs
> @@ -0,0 +1,191 @@
> +// Copyright 2024 Yiyang Wu
> +// SPDX-License-Identifier: MIT or GPL-2.0-or-later
> +
> +#[repr(i32)]
> +#[non_exhaustive]
> +#[allow(clippy::upper_case_acronyms)]
> +#[derive(Debug, Copy, Clone, PartialEq)]
> +pub(crate) enum Errno {
> +    NONE = 0,

Why is NONE an error? No "error: operation completed successfully"
please.

> +    EPERM,
> +    ENOENT,
> +    ESRCH,
> +    EINTR,
> +    EIO,
> +    ENXIO,
> +    E2BIG,
> +    ENOEXEC,
> +    EBADF,
> +    ECHILD,
> +    EAGAIN,
> +    ENOMEM,
> +    EACCES,
> +    EFAULT,
> +    ENOTBLK,
> +    EBUSY,
> +    EEXIST,
> +    EXDEV,
> +    ENODEV,
> +    ENOTDIR,
> +    EISDIR,
> +    EINVAL,
> +    ENFILE,
> +    EMFILE,
> +    ENOTTY,
> +    ETXTBSY,
> +    EFBIG,
> +    ENOSPC,
> +    ESPIPE,
> +    EROFS,
> +    EMLINK,
> +    EPIPE,
> +    EDOM,
> +    ERANGE,
> +    EDEADLK,
> +    ENAMETOOLONG,
> +    ENOLCK,
> +    ENOSYS,
> +    ENOTEMPTY,
> +    ELOOP,
> +    ENOMSG = 42,

This looks very fragile way to maintain an enum.

> +    EIDRM,
> +    ECHRNG,
> +    EL2NSYNC,
> +    EL3HLT,
> +    EL3RST,
> +    ELNRNG,
> +    EUNATCH,
> +    ENOCSI,
> +    EL2HLT,
> +    EBADE,
> +    EBADR,
> +    EXFULL,
> +    ENOANO,
> +    EBADRQC,
> +    EBADSLT,
> +    EBFONT = 59,
> +    ENOSTR,
> +    ENODATA,
> +    ETIME,
> +    ENOSR,
> +    ENONET,
> +    ENOPKG,
> +    EREMOTE,
> +    ENOLINK,
> +    EADV,
> +    ESRMNT,
> +    ECOMM,
> +    EPROTO,
> +    EMULTIHOP,
> +    EDOTDOT,
> +    EBADMSG,
> +    EOVERFLOW,
> +    ENOTUNIQ,
> +    EBADFD,
> +    EREMCHG,
> +    ELIBACC,
> +    ELIBBAD,
> +    ELIBSCN,
> +    ELIBMAX,
> +    ELIBEXEC,
> +    EILSEQ,
> +    ERESTART,
> +    ESTRPIPE,
> +    EUSERS,
> +    ENOTSOCK,
> +    EDESTADDRREQ,
> +    EMSGSIZE,
> +    EPROTOTYPE,
> +    ENOPROTOOPT,
> +    EPROTONOSUPPORT,
> +    ESOCKTNOSUPPORT,
> +    EOPNOTSUPP,
> +    EPFNOSUPPORT,
> +    EAFNOSUPPORT,
> +    EADDRINUSE,
> +    EADDRNOTAVAIL,
> +    ENETDOWN,
> +    ENETUNREACH,
> +    ENETRESET,
> +    ECONNABORTED,
> +    ECONNRESET,
> +    ENOBUFS,
> +    EISCONN,
> +    ENOTCONN,
> +    ESHUTDOWN,
> +    ETOOMANYREFS,
> +    ETIMEDOUT,
> +    ECONNREFUSED,
> +    EHOSTDOWN,
> +    EHOSTUNREACH,
> +    EALREADY,
> +    EINPROGRESS,
> +    ESTALE,
> +    EUCLEAN,
> +    ENOTNAM,
> +    ENAVAIL,
> +    EISNAM,
> +    EREMOTEIO,
> +    EDQUOT,
> +    ENOMEDIUM,
> +    EMEDIUMTYPE,
> +    ECANCELED,
> +    ENOKEY,
> +    EKEYEXPIRED,
> +    EKEYREVOKED,
> +    EKEYREJECTED,
> +    EOWNERDEAD,
> +    ENOTRECOVERABLE,
> +    ERFKILL,
> +    EHWPOISON,
> +    EUNKNOWN,
> +}
> +
> +impl From<i32> for Errno {
> +    fn from(value: i32) -> Self {
> +        if (-value) <= 0 || (-value) > Errno::EUNKNOWN as i32 {
> +            Errno::EUNKNOWN
> +        } else {
> +            // Safety: The value is guaranteed to be a valid errno and the memory
> +            // layout is the same for both types.
> +            unsafe { core::mem::transmute(value) }

This is just unsound. As evident from the fact that you need to manually
specify a few constants, the errno enum doesn't cover all values from 1
to EUNKNOWN.

> +        }
> +    }
> +}
> +
> +impl From<Errno> for i32 {
> +    fn from(value: Errno) -> Self {
> +        -(value as i32)
> +    }
> +}
> +
> +/// Replacement for ERR_PTR in Linux Kernel.
> +impl From<Errno> for *const core::ffi::c_void {
> +    fn from(value: Errno) -> Self {
> +        (-(value as core::ffi::c_long)) as *const core::ffi::c_void
> +    }
> +}
> +
> +impl From<Errno> for *mut core::ffi::c_void {
> +    fn from(value: Errno) -> Self {
> +        (-(value as core::ffi::c_long)) as *mut core::ffi::c_void
> +    }
> +}
> +
> +/// Replacement for PTR_ERR in Linux Kernel.
> +impl From<*const core::ffi::c_void> for Errno {
> +    fn from(value: *const core::ffi::c_void) -> Self {
> +        (-(value as i32)).into()
> +    }
> +}
> +
> +impl From<*mut core::ffi::c_void> for Errno {
> +    fn from(value: *mut core::ffi::c_void) -> Self {
> +        (-(value as i32)).into()
> +    }
> +}
> +/// Replacement for IS_ERR in Linux Kernel.
> +#[inline(always)]
> +pub(crate) fn is_value_err(value: *const core::ffi::c_void) -> bool {
> +    (value as core::ffi::c_ulong) >= (-4095 as core::ffi::c_long) as core::ffi::c_ulong
> +}

Powered by blists - more mailing lists