On Mon, 16 Sep 2024 21:56:13 +0800
Yiyang Wu <toolmanp@xxxxxxx> 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@xxxxxxx>
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
> +}
