Message-ID: <20250910154746.03a3bc2f@nimda.home>
Date: Wed, 10 Sep 2025 15:47:46 +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 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`.

Regards,
Onur

> 
> > > > diff --git a/rust/kernel/miscdevice.rs
> > > > b/rust/kernel/miscdevice.rs index 6373fe183b27..22b72ae84c03
> > > > 100644 --- a/rust/kernel/miscdevice.rs
> > > > +++ b/rust/kernel/miscdevice.rs
> > > > @@ -79,7 +79,7 @@ pub fn register(opts: MiscDeviceOptions) ->
> > > > impl PinInit<Self, Error> { // the destructor of this type
> > > > deallocates the memory. // INVARIANT: If this returns `Ok(())`,
> > > > then the `slot` will contain a registered // misc device.
> > > > -                to_result(unsafe {
> > > > bindings::misc_register(slot) })
> > > > +                to_result(unsafe {
> > > > bindings::misc_register(slot) }).map(|_| ())
> > >
> > > This still uses the `map` pattern. Please change it too.
> > >
> > > Alice
> >
> >
> > I left that part intentionally and explained the reason in v2
> > changelog.
> 
> I missed that, ok. I still prefer Ok::<...>()

Powered by blists - more mailing lists