| Message ID | 20240916135634.98554-4-toolmanp@tlmp.cc (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | erofs: introduce Rust implementation | expand |
On Mon, Sep 16, 2024 at 09:56:13PM +0800, Yiyang Wu 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. Why not just add the missing errno to the core rust code instead? No need to define a whole new one for this. thanks, greg k-h
On Mon, 16 Sep 2024 21:56:13 +0800 Yiyang Wu <toolmanp@tlmp.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@tlmp.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 > +}
Hi Greg, On 2024/9/17 01:51, Greg KH wrote: > On Mon, Sep 16, 2024 at 09:56:13PM +0800, Yiyang Wu 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. > > Why not just add the missing errno to the core rust code instead? No > need to define a whole new one for this. I've discussed with Yiyang about this last week. I also tend to avoid our own errno. The main reason is that Rust errno misses EUCLEAN error number. TBH, I don't know why not just introduces all kernel supported errnos for Rust in one shot. I guess just because no Rust user uses other errno? But for errno cases, I think it's odd for users to add their own errno. Thanks, Gao Xiang > > thanks, > > greg k-h
Hi Gary, On 2024/9/17 04:01, Gary Guo wrote: > On Mon, 16 Sep 2024 21:56:13 +0800 > Yiyang Wu <toolmanp@tlmp.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@tlmp.cc> > > As Greg said, please add missing errno that you need to kernel crate > instead. I've answered Greg about this in another email. > > 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 No, I'd like to try to replace some EROFS C logic first to Rust (by using EROFS C API interfaces) and try if Rust is really useful for a real in-tree filesystem. If Rust can improve EROFS security or performance (although I'm sceptical on performance), As an EROFS maintainer, I'm totally fine to accept EROFS Rust logic landed to help the whole filesystem better. For Rust VFS abstraction, that is a different and indepenent story, Yiyang don't have any bandwidth on this due to his limited time. And I _also_ don't think an incomplete ROFS VFS Rust abstraction is useful to Linux community (because IMO for generic interface design, we need a global vision for all filesystems instead of just ROFSes. No existing user is not an excuse for an incomplete abstraction.) If a reasonble Rust VFS abstraction landed, I think we will switch to use that, but as I said, they are completely two stories. > 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. This patch can be avoided if EUCLEAN is added to errno. Thanks, Gao Xiang
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, + 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, + 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) } + } + } +} + +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 +}
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@tlmp.cc> --- 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