Message-ID: <CAH5fLgjT3LLpg-CjdbR5_+sgBZEsrCP-c4eJt=E3NmtVDKMJ5Q@mail.gmail.com>
Date: Wed, 10 Sep 2025 14:50:03 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Onur Özkan <work@...rozkan.dev>
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, 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

Powered by blists - more mailing lists