Message-ID: <20250910135848.1ea11519@nimda.home>
Date: Wed, 10 Sep 2025 13:58:48 +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 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.

> > 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.

Thanks,
Onur

Powered by blists - more mailing lists