Message-ID: <20250910155558.36ebf8df@nimda.home>
Date: Wed, 10 Sep 2025 15:55:58 +0300
From: Onur Özkan <work@...rozkan.dev>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
 daniel@...lak.dev, dirk.behme@...bosch.com, felipe_life@...e.com,
 tamird@...il.com, dakr@...nel.org, tmgross@...ch.edu,
 a.hindborg@...nel.org, lossin@...nel.org, bjorn3_gh@...tonmail.com,
 gary@...yguo.net, boqun.feng@...il.com, alex.gaynor@...il.com,
 ojeda@...nel.org
Subject: Re: [PATCH v2 1/1] rust: refactor to_result to return the original
 value

On Wed, 10 Sep 2025 14:50:03 +0200
Alice Ryhl <aliceryhl@...gle.com> wrote:

> On Wed, Sep 10, 2025 at 2:47 PM Onur Özkan <work@...rozkan.dev> wrote:
> >
> > On Wed, 10 Sep 2025 13:05:42 +0200
> > Alice Ryhl <aliceryhl@...gle.com> wrote:
> >
> > > On Wed, Sep 10, 2025 at 12:59 PM Onur Özkan <work@...rozkan.dev>
> > > wrote:
> > > >
> > > > On Wed, 10 Sep 2025 06:26:27 +0000
> > > > Alice Ryhl <aliceryhl@...gle.com> wrote:
> > > >
> > > > > On Tue, Sep 09, 2025 at 08:00:13PM +0300, Onur Özkan wrote:
> > > > > > Current `to_result` helper takes a `c_int` and returns
> > > > > > `Ok(())` on success and this has some issues like:
> > > > > >
> > > > > >     - Callers lose the original return value and often have
> > > > > > to store it in a temporary variable before calling
> > > > > > `to_result`.
> > > > > >
> > > > > >     - It only supports `c_int`, which makes callers to
> > > > > > unnecessarily cast when working with other types (e.g.
> > > > > > `u16` in phy abstractions). We even have some places that
> > > > > > ignore to use `to_result` helper because the input doesn't
> > > > > > fit in `c_int` (see [0]).
> > > > > >
> > > > > > [0]:
> > > > > > https://lore.kernel.org/all/20250822080252.773d6f54@nimda.home/
> > > > > >
> > > > > > This patch changes `to_result` to be generic and also
> > > > > > return the original value on success.
> > > > > >
> > > > > > So that the code that previously looked like:
> > > > > >
> > > > > >     let ret = unsafe { bindings::some_ffi_call() };
> > > > > >     to_result(ret).map(|()| SomeType::new(ret))
> > > > > >
> > > > > > can now be written more directly as:
> > > > > >
> > > > > >     to_result(unsafe { bindings::some_ffi_call() })
> > > > > >     .map(|ret| SomeType::new(ret))
> > > > > >
> > > > > > Similarly, code such as:
> > > > > >
> > > > > >     let res: isize = $some_ffi_call();
> > > > > >     if res < 0 {
> > > > > >         return Err(Error::from_errno(res as i32));
> > > > > >     }
> > > > > >
> > > > > > can now be used with `to_result` as:
> > > > > >
> > > > > >     to_result($some_ffi_call())?;
> > > > > >
> > > > > > This patch only fixes the callers that broke after the
> > > > > > changes on `to_result`. I haven't included all the
> > > > > > improvements made possible by the new design since that
> > > > > > could conflict with other ongoing patches [1]. Once this
> > > > > > patch is approved and applied, I am planning to follow up
> > > > > > with creating a "good first issue" on [2] for those
> > > > > > additional changes.
> > > > > >
> > > > > > [1]: https://lore.kernel.org/rust-for-linux/?q=to_result
> > > > > > [2]: https://github.com/Rust-for-Linux/linux
> > > > > >
> > > > > > Link:
> > > > > > https://rust-for-linux.zulipchat.com/#narrow/channel/288089/topic/x/near/536374456
> > > > > > Signed-off-by: Onur Özkan <work@...rozkan.dev>
> > > > >
> > > > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > > > index a41de293dcd1..6563ea71e203 100644
> > > > > > --- a/rust/kernel/error.rs
> > > > > > +++ b/rust/kernel/error.rs
> > > > > > @@ -376,12 +376,19 @@ fn from(e: core::convert::Infallible)
> > > > > > -> Error { pub type Result<T = (), E = Error> =
> > > > > > core::result::Result<T, E>;
> > > > > >
> > > > > >  /// Converts an integer as returned by a C kernel function
> > > > > > to an error if it's negative, and -/// `Ok(())` otherwise.
> > > > > > -pub fn to_result(err: crate::ffi::c_int) -> Result {
> > > > > > -    if err < 0 {
> > > > > > -        Err(Error::from_errno(err))
> > > > > > +/// returns the original value otherwise.
> > > > > > +pub fn to_result<T>(code: T) -> Result<T>
> > > > > > +where
> > > > > > +    T: Copy + TryInto<i32>,
> > > > > > +{
> > > > > > +    // Try casting into `i32`.
> > > > > > +    let casted: crate::ffi::c_int =
> > > > > > code.try_into().unwrap_or(0); +
> > > > > > +    if casted < 0 {
> > > > > > +        Err(Error::from_errno(casted))
> > > > > >      } else {
> > > > > > -        Ok(())
> > > > > > +        // Return the original input value.
> > > > > > +        Ok(code)
> > > > > >      }
> > > > > >  }
> > > > >
> > > > > I don't think this is the best way to declare this function.
> > > > > The conversions I would want are:
> > > > >
> > > > > * i32 -> Result<u32>
> > > > > * isize -> Result<usize>
> > > > > * i64 -> Result<u64>
> > > > >
> > > > > Your commit messages mentions i16, but does the error types
> > > > > even fit in 16 bits? Maybe. But they don't fit in i8. That is
> > > > > to say, I think it should support all the types larger than
> > > > > i32 (the errors fit in those types too), but for the ones
> > > > > that are smaller, it might not make sense if the type is too
> > > > > small. That's the reverse of what you have now.
> > > > >
> > > > > We probably need a new trait. E.g.:
> > > > >
> > > > > trait ToResult {
> > > > >     type Unsigned;
> > > > >     fn to_result(self) -> Result<Self::Unsigned, Error>;
> > > > > }
> > > > >
> > > > > impl ToResult for i32 {
> > > > >     type Unsigned = u32;
> > > > >     fn to_result(self) -> Result<u32, Error> {
> > > > >         ...
> > > > >     }
> > > > > }
> > > > >
> > > > > impl ToResult for isize {
> > > > >     type Unsigned = usize;
> > > > >     fn to_result(self) -> Result<usize, Error> {
> > > > >         ...
> > > > >     }
> > > > > }
> > > > >
> > > > > pub fn to_result<T: ToResult>(code: T) -> Result<T::Unsigned>
> > > > > { T::to_result(code)
> > > > > }
> > > > >
> > > >
> > > > `Error::from_errno` is limited to i32, that's why I followed the
> > > > `TryInto<i32>` approach, but I like this design too.
> > >
> > > If you pass an i32 that is not a valid errno, then it becomes
> > > EINVAL. In the case of `isize`, I would say that if a negative
> > > isize does not fit in i32, then that should fall into the same
> > > scenario.
> > >
> >
> > In that case replacing `unwrap_or(0)` with `map_err(|_|
> > code::EINVAL)` should do the job?
> >
> > I also thought of an alternative to the custom-trait–based approach.
> > What do you think about something like this:
> >
> >     pub fn to_result<T, R>(code: T) -> Result<R>
> >     where
> >         T: Copy + TryInto<i32> + TryInto<R>,
> >     {
> >         // Try casting into `i32`.
> >         let casted: crate::ffi::c_int = code.try_into().map_err(|_|
> >     code::EINVAL)?;
> >
> >         if casted < 0 {
> >             Err(Error::from_errno(casted))
> >         } else {
> >             // Return the original input value as `R`.
> >             code.try_into().map_err(|_| code::EINVAL)
> >         }
> >     }
> >
> >
> > On the caller side, it would look like this:
> >
> >     let val: u16 = to_result(...)?;
> >
> > The main difference here is that this version can be used to cast
> > into multiple different types, not just `i32 -> u32` or `i64 ->
> > u64`.
> 
> I think making the return type a separate generic makes this too
> difficult to use. It means that any time you would write this:
> 
> to_result(unsafe { ... })?;
> Ok(())
> 
> now you need to tell the compiler what kind of integer you want to get
> from to_result, just so you can immediately ignore the integer.
> 
> Alice

Yes, and with the custom trait you need to import it in order to use
the `to_result` helper. I don't have a strong preference either way.
I guess I will wait a couple of days to get more feedback from others
as well. Thank you for your quick feedback and review so far!

-Onur

Powered by blists - more mailing lists