From: Wedson Almeida Filho <wedsonaf@gmail.com>

Add a helper macro to easily return C result codes from a Rust function
that calls functions which return a Result<T>.

Lina: Imported from rust-for-linux/rust, originally developed by Wedson
as part of file_operations.rs. Added the allow() flags since there is no
user in the kernel crate yet and fixed a typo in a comment.

Co-developed-by: Fox Chen <foxhlchen@gmail.com>
Signed-off-by: Fox Chen <foxhlchen@gmail.com>
Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index cf3d089477d2..8a9222595cd1 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
     }
     Ok(ptr)
 }
+
+// TODO: Remove `dead_code` marker once an in-kernel client is available.
+#[allow(dead_code)]
+pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
+where
+    T: From<i16>,
+{
+    match r {
+        Ok(v) => v,
+        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
+        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
+        // therefore a negative `errno` always fits in an `i16` and will not overflow.
+        Err(e) => T::from(e.to_kernel_errno() as i16),
+    }
+}
+
+/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
+///
+/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
+/// from inside `extern "C"` functions that need to return an integer
+/// error result.
+///
+/// `T` should be convertible from an `i16` via `From<i16>`.
+///
+/// # Examples
+///
+/// ```ignore
+/// # use kernel::from_kernel_result;
+/// # use kernel::bindings;
+/// unsafe extern "C" fn probe_callback(
+///     pdev: *mut bindings::platform_device,
+/// ) -> core::ffi::c_int {
+///     from_kernel_result! {
+///         let ptr = devm_alloc(pdev)?;
+///         bindings::platform_set_drvdata(pdev, ptr);
+///         Ok(0)
+///     }
+/// }
+/// ```
+// TODO: Remove `unused_macros` marker once an in-kernel client is available.
+#[allow(unused_macros)]
+macro_rules! from_kernel_result {
+    ($($tt:tt)*) => {{
+        $crate::error::from_kernel_result_helper((|| {
+            $($tt)*
+        })())
+    }};
+}
+
+// TODO: Remove `unused_imports` marker once an in-kernel client is available.
+#[allow(unused_imports)]
+pub(crate) use from_kernel_result;

-- 
2.35.1

On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> Add a helper macro to easily return C result codes from a Rust function
> that calls functions which return a Result<T>.
> 
> Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> as part of file_operations.rs. Added the allow() flags since there is no
> user in the kernel crate yet and fixed a typo in a comment.
> 
> Co-developed-by: Fox Chen <foxhlchen@gmail.com>
> Signed-off-by: Fox Chen <foxhlchen@gmail.com>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>  rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index cf3d089477d2..8a9222595cd1 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
>      }
>      Ok(ptr)
>  }
> +
> +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> +#[allow(dead_code)]
> +pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
> +where
> +    T: From<i16>,
> +{
> +    match r {
> +        Ok(v) => v,
> +        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
> +        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
> +        // therefore a negative `errno` always fits in an `i16` and will not overflow.
> +        Err(e) => T::from(e.to_kernel_errno() as i16),
> +    }
> +}
> +
> +/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
> +///
> +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
> +/// from inside `extern "C"` functions that need to return an integer
> +/// error result.
> +///
> +/// `T` should be convertible from an `i16` via `From<i16>`.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// # use kernel::from_kernel_result;
> +/// # use kernel::bindings;
> +/// unsafe extern "C" fn probe_callback(
> +///     pdev: *mut bindings::platform_device,
> +/// ) -> core::ffi::c_int {
> +///     from_kernel_result! {
> +///         let ptr = devm_alloc(pdev)?;
> +///         bindings::platform_set_drvdata(pdev, ptr);
> +///         Ok(0)
> +///     }
> +/// }
> +/// ```
> +// TODO: Remove `unused_macros` marker once an in-kernel client is available.
> +#[allow(unused_macros)]
> +macro_rules! from_kernel_result {

This actually doesn't need to be a macro, right? The following function
version:

	pub fn from_kernel_result<T, F>(f: F) -> T
	where
	    T: From<i16>,
	    F: FnOnce() -> Result<T>;

is not bad, the above case then can be written as:

	unsafe extern "C" fn probe_callback(
	    pdev: *mut bindings::platform_device,
	) -> core::ffi::c_int {
	    from_kernel_result(|| {
		let ptr = devm_alloc(pdev)?;
		bindings::platform_set_drvdata(pdev, ptr);
		Ok(0)
	    })
	}

less magical, but the control flow is more clear.

Thoughts?

Regards,
Boqun

> +    ($($tt:tt)*) => {{
> +        $crate::error::from_kernel_result_helper((|| {
> +            $($tt)*
> +        })())
> +    }};
> +}
> +
> +// TODO: Remove `unused_imports` marker once an in-kernel client is available.
> +#[allow(unused_imports)]
> +pub(crate) use from_kernel_result;
> 
> -- 
> 2.35.1
>

On Fri, 24 Feb 2023 15:56:05 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > 
> > Add a helper macro to easily return C result codes from a Rust function
> > that calls functions which return a Result<T>.
> > 
> > Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> > as part of file_operations.rs. Added the allow() flags since there is no
> > user in the kernel crate yet and fixed a typo in a comment.
> > 
> > Co-developed-by: Fox Chen <foxhlchen@gmail.com>
> > Signed-off-by: Fox Chen <foxhlchen@gmail.com>
> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > ---
> >  rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> > 
> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > index cf3d089477d2..8a9222595cd1 100644
> > --- a/rust/kernel/error.rs
> > +++ b/rust/kernel/error.rs
> > @@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
> >      }
> >      Ok(ptr)
> >  }
> > +
> > +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> > +#[allow(dead_code)]
> > +pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
> > +where
> > +    T: From<i16>,
> > +{
> > +    match r {
> > +        Ok(v) => v,
> > +        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
> > +        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
> > +        // therefore a negative `errno` always fits in an `i16` and will not overflow.
> > +        Err(e) => T::from(e.to_kernel_errno() as i16),
> > +    }
> > +}
> > +
> > +/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
> > +///
> > +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
> > +/// from inside `extern "C"` functions that need to return an integer
> > +/// error result.
> > +///
> > +/// `T` should be convertible from an `i16` via `From<i16>`.
> > +///
> > +/// # Examples
> > +///
> > +/// ```ignore
> > +/// # use kernel::from_kernel_result;
> > +/// # use kernel::bindings;
> > +/// unsafe extern "C" fn probe_callback(
> > +///     pdev: *mut bindings::platform_device,
> > +/// ) -> core::ffi::c_int {
> > +///     from_kernel_result! {
> > +///         let ptr = devm_alloc(pdev)?;
> > +///         bindings::platform_set_drvdata(pdev, ptr);
> > +///         Ok(0)
> > +///     }
> > +/// }
> > +/// ```
> > +// TODO: Remove `unused_macros` marker once an in-kernel client is available.
> > +#[allow(unused_macros)]
> > +macro_rules! from_kernel_result {  
> 
> This actually doesn't need to be a macro, right? The following function
> version:
> 
> 	pub fn from_kernel_result<T, F>(f: F) -> T
> 	where
> 	    T: From<i16>,
> 	    F: FnOnce() -> Result<T>;
> 
> is not bad, the above case then can be written as:
> 
> 	unsafe extern "C" fn probe_callback(
> 	    pdev: *mut bindings::platform_device,
> 	) -> core::ffi::c_int {
> 	    from_kernel_result(|| {
> 		let ptr = devm_alloc(pdev)?;
> 		bindings::platform_set_drvdata(pdev, ptr);
> 		Ok(0)
> 	    })
> 	}
> 
> less magical, but the control flow is more clear.
> 
> Thoughts?

I don't think even the closure is necessary? Why not just

 	pub fn from_kernel_result<T: From<i16>>(r: Result<T>) -> T

and

 	unsafe extern "C" fn probe_callback(
 	    pdev: *mut bindings::platform_device,
 	) -> core::ffi::c_int {
 	    from_kernel_result({
 		let ptr = devm_alloc(pdev)?;
 		bindings::platform_set_drvdata(pdev, ptr);
 		Ok(0)
 	    })
 	}

?

Best,
Gary

On Sat, Feb 25, 2023 at 10:23:40PM +0000, Gary Guo wrote:
> On Fri, 24 Feb 2023 15:56:05 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > 
> > > Add a helper macro to easily return C result codes from a Rust function
> > > that calls functions which return a Result<T>.
> > > 
> > > Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> > > as part of file_operations.rs. Added the allow() flags since there is no
> > > user in the kernel crate yet and fixed a typo in a comment.
> > > 
> > > Co-developed-by: Fox Chen <foxhlchen@gmail.com>
> > > Signed-off-by: Fox Chen <foxhlchen@gmail.com>
> > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > > ---
> > >  rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 52 insertions(+)
> > > 
> > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > index cf3d089477d2..8a9222595cd1 100644
> > > --- a/rust/kernel/error.rs
> > > +++ b/rust/kernel/error.rs
> > > @@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
> > >      }
> > >      Ok(ptr)
> > >  }
> > > +
> > > +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> > > +#[allow(dead_code)]
> > > +pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
> > > +where
> > > +    T: From<i16>,
> > > +{
> > > +    match r {
> > > +        Ok(v) => v,
> > > +        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
> > > +        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
> > > +        // therefore a negative `errno` always fits in an `i16` and will not overflow.
> > > +        Err(e) => T::from(e.to_kernel_errno() as i16),
> > > +    }
> > > +}
> > > +
> > > +/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
> > > +///
> > > +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
> > > +/// from inside `extern "C"` functions that need to return an integer
> > > +/// error result.
> > > +///
> > > +/// `T` should be convertible from an `i16` via `From<i16>`.
> > > +///
> > > +/// # Examples
> > > +///
> > > +/// ```ignore
> > > +/// # use kernel::from_kernel_result;
> > > +/// # use kernel::bindings;
> > > +/// unsafe extern "C" fn probe_callback(
> > > +///     pdev: *mut bindings::platform_device,
> > > +/// ) -> core::ffi::c_int {
> > > +///     from_kernel_result! {
> > > +///         let ptr = devm_alloc(pdev)?;
> > > +///         bindings::platform_set_drvdata(pdev, ptr);
> > > +///         Ok(0)
> > > +///     }
> > > +/// }
> > > +/// ```
> > > +// TODO: Remove `unused_macros` marker once an in-kernel client is available.
> > > +#[allow(unused_macros)]
> > > +macro_rules! from_kernel_result {  
> > 
> > This actually doesn't need to be a macro, right? The following function
> > version:
> > 
> > 	pub fn from_kernel_result<T, F>(f: F) -> T
> > 	where
> > 	    T: From<i16>,
> > 	    F: FnOnce() -> Result<T>;
> > 
> > is not bad, the above case then can be written as:
> > 
> > 	unsafe extern "C" fn probe_callback(
> > 	    pdev: *mut bindings::platform_device,
> > 	) -> core::ffi::c_int {
> > 	    from_kernel_result(|| {
> > 		let ptr = devm_alloc(pdev)?;
> > 		bindings::platform_set_drvdata(pdev, ptr);
> > 		Ok(0)
> > 	    })
> > 	}
> > 
> > less magical, but the control flow is more clear.
> > 
> > Thoughts?
> 
> I don't think even the closure is necessary? Why not just
> 
>  	pub fn from_kernel_result<T: From<i16>>(r: Result<T>) -> T
> 
> and
> 
>  	unsafe extern "C" fn probe_callback(
>  	    pdev: *mut bindings::platform_device,
>  	) -> core::ffi::c_int {
>  	    from_kernel_result({
>  		let ptr = devm_alloc(pdev)?;

Hmm.. looks like the `?` only "propagating them (the errors) to the
calling *function*":

	https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator

, so this doesn't work as we expect:

	https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=64c9039e1da2c436f9f4f5ea46e97e90

Hope I didn't miss something subtle here..

Regards,
Boqun

>  		bindings::platform_set_drvdata(pdev, ptr);
>  		Ok(0)
>  	    })
>  	}
> 
> ?
> 
> Best,
> Gary

On Sun, Feb 26, 2023 at 3:22 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hmm.. looks like the `?` only "propagating them (the errors) to the
> calling *function*":
>
>         https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator
>
> , so this doesn't work as we expect:
>
>         https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=64c9039e1da2c436f9f4f5ea46e97e90
>
> Hope I didn't miss something subtle here..

There is `feature(try_blocks)` [1] for that:

    https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=8f40663d2ad51e04a13f2da5c4580fc0

[1] https://github.com/rust-lang/rust/issues/31436.

Cheers,
Miguel

On Sat, 25 Feb 2023 18:22:11 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Sat, Feb 25, 2023 at 10:23:40PM +0000, Gary Guo wrote:
> > On Fri, 24 Feb 2023 15:56:05 -0800
> > Boqun Feng <boqun.feng@gmail.com> wrote:
> >   
> > > On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:  
> > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > 
> > > > Add a helper macro to easily return C result codes from a Rust function
> > > > that calls functions which return a Result<T>.
> > > > 
> > > > Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> > > > as part of file_operations.rs. Added the allow() flags since there is no
> > > > user in the kernel crate yet and fixed a typo in a comment.
> > > > 
> > > > Co-developed-by: Fox Chen <foxhlchen@gmail.com>
> > > > Signed-off-by: Fox Chen <foxhlchen@gmail.com>
> > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > > > ---
> > > >  rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 52 insertions(+)
> > > > 
> > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > index cf3d089477d2..8a9222595cd1 100644
> > > > --- a/rust/kernel/error.rs
> > > > +++ b/rust/kernel/error.rs
> > > > @@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
> > > >      }
> > > >      Ok(ptr)
> > > >  }
> > > > +
> > > > +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> > > > +#[allow(dead_code)]
> > > > +pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
> > > > +where
> > > > +    T: From<i16>,
> > > > +{
> > > > +    match r {
> > > > +        Ok(v) => v,
> > > > +        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
> > > > +        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
> > > > +        // therefore a negative `errno` always fits in an `i16` and will not overflow.
> > > > +        Err(e) => T::from(e.to_kernel_errno() as i16),
> > > > +    }
> > > > +}
> > > > +
> > > > +/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
> > > > +///
> > > > +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
> > > > +/// from inside `extern "C"` functions that need to return an integer
> > > > +/// error result.
> > > > +///
> > > > +/// `T` should be convertible from an `i16` via `From<i16>`.
> > > > +///
> > > > +/// # Examples
> > > > +///
> > > > +/// ```ignore
> > > > +/// # use kernel::from_kernel_result;
> > > > +/// # use kernel::bindings;
> > > > +/// unsafe extern "C" fn probe_callback(
> > > > +///     pdev: *mut bindings::platform_device,
> > > > +/// ) -> core::ffi::c_int {
> > > > +///     from_kernel_result! {
> > > > +///         let ptr = devm_alloc(pdev)?;
> > > > +///         bindings::platform_set_drvdata(pdev, ptr);
> > > > +///         Ok(0)
> > > > +///     }
> > > > +/// }
> > > > +/// ```
> > > > +// TODO: Remove `unused_macros` marker once an in-kernel client is available.
> > > > +#[allow(unused_macros)]
> > > > +macro_rules! from_kernel_result {    
> > > 
> > > This actually doesn't need to be a macro, right? The following function
> > > version:
> > > 
> > > 	pub fn from_kernel_result<T, F>(f: F) -> T
> > > 	where
> > > 	    T: From<i16>,
> > > 	    F: FnOnce() -> Result<T>;
> > > 
> > > is not bad, the above case then can be written as:
> > > 
> > > 	unsafe extern "C" fn probe_callback(
> > > 	    pdev: *mut bindings::platform_device,
> > > 	) -> core::ffi::c_int {
> > > 	    from_kernel_result(|| {
> > > 		let ptr = devm_alloc(pdev)?;
> > > 		bindings::platform_set_drvdata(pdev, ptr);
> > > 		Ok(0)
> > > 	    })
> > > 	}
> > > 
> > > less magical, but the control flow is more clear.
> > > 
> > > Thoughts?  
> > 
> > I don't think even the closure is necessary? Why not just
> > 
> >  	pub fn from_kernel_result<T: From<i16>>(r: Result<T>) -> T
> > 
> > and
> > 
> >  	unsafe extern "C" fn probe_callback(
> >  	    pdev: *mut bindings::platform_device,
> >  	) -> core::ffi::c_int {
> >  	    from_kernel_result({
> >  		let ptr = devm_alloc(pdev)?;  
> 
> Hmm.. looks like the `?` only "propagating them (the errors) to the
> calling *function*":
> 
> 	https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator
> 
> , so this doesn't work as we expect:
> 
> 	https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=64c9039e1da2c436f9f4f5ea46e97e90

Oh, you're absolutely right, I guess I shouldn't be doing code review
in the middle of the night...

However, if we have the try blocks then we should be able to just
rewrite it as

	from_kernel_result(try {
	    ...
	})

I guess in this sense it might worth keeping this as a macro so we can
tweak the implementation from closure to try blocks without having to
edit all use sites?

Best,
Gary

On Sun, Feb 26, 2023 at 01:36:06PM +0000, Gary Guo wrote:
> On Sat, 25 Feb 2023 18:22:11 -0800
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Sat, Feb 25, 2023 at 10:23:40PM +0000, Gary Guo wrote:
> > > On Fri, 24 Feb 2023 15:56:05 -0800
> > > Boqun Feng <boqun.feng@gmail.com> wrote:
> > >   
> > > > On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:  
> > > > > From: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > > 
> > > > > Add a helper macro to easily return C result codes from a Rust function
> > > > > that calls functions which return a Result<T>.
> > > > > 
> > > > > Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> > > > > as part of file_operations.rs. Added the allow() flags since there is no
> > > > > user in the kernel crate yet and fixed a typo in a comment.
> > > > > 
> > > > > Co-developed-by: Fox Chen <foxhlchen@gmail.com>
> > > > > Signed-off-by: Fox Chen <foxhlchen@gmail.com>
> > > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> > > > > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > > > > ---
> > > > >  rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 52 insertions(+)
> > > > > 
> > > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > > index cf3d089477d2..8a9222595cd1 100644
> > > > > --- a/rust/kernel/error.rs
> > > > > +++ b/rust/kernel/error.rs
> > > > > @@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
> > > > >      }
> > > > >      Ok(ptr)
> > > > >  }
> > > > > +
> > > > > +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> > > > > +#[allow(dead_code)]
> > > > > +pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
> > > > > +where
> > > > > +    T: From<i16>,
> > > > > +{
> > > > > +    match r {
> > > > > +        Ok(v) => v,
> > > > > +        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
> > > > > +        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
> > > > > +        // therefore a negative `errno` always fits in an `i16` and will not overflow.
> > > > > +        Err(e) => T::from(e.to_kernel_errno() as i16),
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
> > > > > +///
> > > > > +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
> > > > > +/// from inside `extern "C"` functions that need to return an integer
> > > > > +/// error result.
> > > > > +///
> > > > > +/// `T` should be convertible from an `i16` via `From<i16>`.
> > > > > +///
> > > > > +/// # Examples
> > > > > +///
> > > > > +/// ```ignore
> > > > > +/// # use kernel::from_kernel_result;
> > > > > +/// # use kernel::bindings;
> > > > > +/// unsafe extern "C" fn probe_callback(
> > > > > +///     pdev: *mut bindings::platform_device,
> > > > > +/// ) -> core::ffi::c_int {
> > > > > +///     from_kernel_result! {
> > > > > +///         let ptr = devm_alloc(pdev)?;
> > > > > +///         bindings::platform_set_drvdata(pdev, ptr);
> > > > > +///         Ok(0)
> > > > > +///     }
> > > > > +/// }
> > > > > +/// ```
> > > > > +// TODO: Remove `unused_macros` marker once an in-kernel client is available.
> > > > > +#[allow(unused_macros)]
> > > > > +macro_rules! from_kernel_result {    
> > > > 
> > > > This actually doesn't need to be a macro, right? The following function
> > > > version:
> > > > 
> > > > 	pub fn from_kernel_result<T, F>(f: F) -> T
> > > > 	where
> > > > 	    T: From<i16>,
> > > > 	    F: FnOnce() -> Result<T>;
> > > > 
> > > > is not bad, the above case then can be written as:
> > > > 
> > > > 	unsafe extern "C" fn probe_callback(
> > > > 	    pdev: *mut bindings::platform_device,
> > > > 	) -> core::ffi::c_int {
> > > > 	    from_kernel_result(|| {
> > > > 		let ptr = devm_alloc(pdev)?;
> > > > 		bindings::platform_set_drvdata(pdev, ptr);
> > > > 		Ok(0)
> > > > 	    })
> > > > 	}
> > > > 
> > > > less magical, but the control flow is more clear.
> > > > 
> > > > Thoughts?  
> > > 
> > > I don't think even the closure is necessary? Why not just
> > > 
> > >  	pub fn from_kernel_result<T: From<i16>>(r: Result<T>) -> T
> > > 
> > > and
> > > 
> > >  	unsafe extern "C" fn probe_callback(
> > >  	    pdev: *mut bindings::platform_device,
> > >  	) -> core::ffi::c_int {
> > >  	    from_kernel_result({
> > >  		let ptr = devm_alloc(pdev)?;  
> > 
> > Hmm.. looks like the `?` only "propagating them (the errors) to the
> > calling *function*":
> > 
> > 	https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator
> > 
> > , so this doesn't work as we expect:
> > 
> > 	https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=64c9039e1da2c436f9f4f5ea46e97e90
> 
> Oh, you're absolutely right, I guess I shouldn't be doing code review
> in the middle of the night...
> 

;-)

> However, if we have the try blocks then we should be able to just
> rewrite it as
> 
> 	from_kernel_result(try {
> 	    ...
> 	})
> 

Thank you and Miguel for the references on try blocks, I vaguely
remember I heard of them, but never take a close look.

> I guess in this sense it might worth keeping this as a macro so we can
> tweak the implementation from closure to try blocks without having to
> edit all use sites?
> 

My preference to function instead of macro here is because I want to
avoid the extra level of abstraction and make things explict, so that
users and reviewers can understand the API behavior solely based on
Rust's types, functions and closures: they are simpler than macros, at
least to me ;-)

Speak of future changes in the implementation:

First, I think the macro version here is just a poor-man's try block, in
other words, I'd expect explicit use of try blocks intead of
`from_kernel_result` when the feature is ready. If that's the case, we
need to change the use sites anyway.

Second, I think the semantics is a little different between try block
implementation and closure implemention? Consider the following case:

	// the outer function return a Result<i32>

	let a = from_kernel_result!({
		...
		return Some(0); // x is i32
		..
	});

	return Some(a + 1);

Do both implementation share the same behavior?

No one can predict the future, but being simple at the beginning sounds
a good strategy to me.

Regards,
Boqun

> Best,
> Gary

On Sun, Feb 26, 2023 at 7:17 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> My preference to function instead of macro here is because I want to
> avoid the extra level of abstraction and make things explict, so that
> users and reviewers can understand the API behavior solely based on
> Rust's types, functions and closures: they are simpler than macros, at
> least to me ;-)

There is one extra problem with the macro: `rustfmt` does not format
the contents if called with braces (as we currently do).

So when I was cleaning some things up for v8, one of the things I did
was run manually `rustfmt` on the blocks by removing the macro
invocation, in commit 77a1a8c952e1 ("rust: kernel: apply `rustfmt` to
`from_kernel_result!` blocks").

Having said that, it does format it when called with parenthesis
wrapping the block, so we could do that if we end up with the macro.

> First, I think the macro version here is just a poor-man's try block, in
> other words, I'd expect explicit use of try blocks intead of
> `from_kernel_result` when the feature is ready. If that's the case, we
> need to change the use sites anyway.

Yeah, if we eventually get a better language feature that fits well,
then we should use it.

> Do both implementation share the same behavior?

Yeah, a `return` will return to the outer caller in the case of a
`try` block, while it returns to the closure (macro) in the other
case. Or do you mean something else?

In that case, I think one could use use a labeled block to `break`
out, not sure if `try` blocks will allow an easier way.

We have a case of such a `return` within the closure at `rust/rust` in
`file.rs`:

    from_kernel_result! {
        let off = match whence as u32 {
            bindings::SEEK_SET => SeekFrom::Start(offset.try_into()?),
            bindings::SEEK_CUR => SeekFrom::Current(offset),
            bindings::SEEK_END => SeekFrom::End(offset),
            _ => return Err(EINVAL),
        };
        ...
        Ok(off as bindings::loff_t)
    }

Cheers,
Miguel

On 2/26/23 17:59, Miguel Ojeda wrote:
> On Sun, Feb 26, 2023 at 7:17 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>>
>> My preference to function instead of macro here is because I want to
>> avoid the extra level of abstraction and make things explict, so that
>> users and reviewers can understand the API behavior solely based on
>> Rust's types, functions and closures: they are simpler than macros, at
>> least to me ;-)
> 
> There is one extra problem with the macro: `rustfmt` does not format
> the contents if called with braces (as we currently do).
> 
> So when I was cleaning some things up for v8, one of the things I did
> was run manually `rustfmt` on the blocks by removing the macro
> invocation, in commit 77a1a8c952e1 ("rust: kernel: apply `rustfmt` to
> `from_kernel_result!` blocks").
> 
> Having said that, it does format it when called with parenthesis
> wrapping the block, so we could do that if we end up with the macro.

Also rust-analyzer can't analyze the insides of a from_kernel_result!
block. Only thing it can do is to suggest a macro expansion. Plus, this
macro triggers a clippy lint on a redundant call on a closure. So it's a
bit annoying to work with.

On Sun, Feb 26, 2023 at 09:59:25PM +0100, Miguel Ojeda wrote:
> On Sun, Feb 26, 2023 at 7:17 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > My preference to function instead of macro here is because I want to
> > avoid the extra level of abstraction and make things explict, so that
> > users and reviewers can understand the API behavior solely based on
> > Rust's types, functions and closures: they are simpler than macros, at
> > least to me ;-)
> 
> There is one extra problem with the macro: `rustfmt` does not format
> the contents if called with braces (as we currently do).

Interesting, sounds like a missing feature in `rustfmt` or maybe we
don't use the correct config ;-)

> 
> So when I was cleaning some things up for v8, one of the things I did
> was run manually `rustfmt` on the blocks by removing the macro
> invocation, in commit 77a1a8c952e1 ("rust: kernel: apply `rustfmt` to
> `from_kernel_result!` blocks").
> 
> Having said that, it does format it when called with parenthesis
> wrapping the block, so we could do that if we end up with the macro.
> 
> > First, I think the macro version here is just a poor-man's try block, in
> > other words, I'd expect explicit use of try blocks intead of
> > `from_kernel_result` when the feature is ready. If that's the case, we
> > need to change the use sites anyway.
> 
> Yeah, if we eventually get a better language feature that fits well,
> then we should use it.
> 
> > Do both implementation share the same behavior?
> 
> Yeah, a `return` will return to the outer caller in the case of a
> `try` block, while it returns to the closure (macro) in the other
> case. Or do you mean something else?
> 

"Yeah" means they have different behaviors, right? ;-)

Thanks for confirming and I think you get it, but just in case for
others reading this: if we use the macro way to implement
`from_kernel_result` as in this patch:

	macro_rules! from_kernel_result {
	    ($($tt:tt)*) => {{
		$crate::error::from_kernel_result_helper((|| {
		    $($tt)*
		})())
	    }};
	}

and later we re-implement with try blocks:

	macro_rules! from_kernel_result {
	    ($($tt:tt)*) => {{
		$crate::error::from_kernel_result_helper(try {
		    $($tt)*
		})
	    }};
	}

the `from_kernel_result` semantics will get changed on the `return`
statement inside the macro blocks.

And this is another reason why we want to avoid use macros here. Code
example as below:

	https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=58ea8b95cdfd6b053561052853b0ac00

`foo_v1` and `foo_v3` has the exact same function body, but behave
differently.

> In that case, I think one could use use a labeled block to `break`
> out, not sure if `try` blocks will allow an easier way.
> 
> We have a case of such a `return` within the closure at `rust/rust` in
> `file.rs`:

Thanks for finding an example! Means we did use return.

For this particular API, I'd say function right now, `try` blocks if
avaiable.

Regards,
Boqun

> 
>     from_kernel_result! {
>         let off = match whence as u32 {
>             bindings::SEEK_SET => SeekFrom::Start(offset.try_into()?),
>             bindings::SEEK_CUR => SeekFrom::Current(offset),
>             bindings::SEEK_END => SeekFrom::End(offset),
>             _ => return Err(EINVAL),
>         };
>         ...
>         Ok(off as bindings::loff_t)
>     }
> 
> Cheers,
> Miguel

On Sun, Feb 26, 2023 at 11:13 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Interesting, sounds like a missing feature in `rustfmt` or maybe we
> don't use the correct config ;-)

It may be coming [1] (I haven't tested if that one would work for us),
but in general it is hard for `rustfmt` because the contents are not
necessarily valid Rust code.

[1] https://github.com/rust-lang/rustfmt/pull/5538

> "Yeah" means they have different behaviors, right? ;-)

Yes, sorry for the confusion :)

> Thanks for finding an example! Means we did use return.
>
> For this particular API, I'd say function right now, `try` blocks if
> avaiable.

Do you mean going with the closure for the time being and `try` blocks
when they become stable? Yeah, I think that is a fair approach.

Cheers,
Miguel

On Mon, Feb 27, 2023 at 01:10:39PM +0100, Miguel Ojeda wrote:
> On Sun, Feb 26, 2023 at 11:13 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Interesting, sounds like a missing feature in `rustfmt` or maybe we
> > don't use the correct config ;-)
> 
> It may be coming [1] (I haven't tested if that one would work for us),
> but in general it is hard for `rustfmt` because the contents are not
> necessarily valid Rust code.
> 
> [1] https://github.com/rust-lang/rustfmt/pull/5538
> 
> > "Yeah" means they have different behaviors, right? ;-)
> 
> Yes, sorry for the confusion :)
> 

No worries, English is the one to blame ;-)

> > Thanks for finding an example! Means we did use return.
> >
> > For this particular API, I'd say function right now, `try` blocks if
> > avaiable.
> 
> Do you mean going with the closure for the time being and `try` blocks
> when they become stable? Yeah, I think that is a fair approach.
> 

Right, and like my original suggestion to Lina, don't use macro for this
one.

Regards,
Boqun

> Cheers,
> Miguel

On 25/02/2023 08.56, Boqun Feng wrote:
> On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
>> From: Wedson Almeida Filho <wedsonaf@gmail.com>
>>
>> Add a helper macro to easily return C result codes from a Rust function
>> that calls functions which return a Result<T>.
>>
>> Lina: Imported from rust-for-linux/rust, originally developed by Wedson
>> as part of file_operations.rs. Added the allow() flags since there is no
>> user in the kernel crate yet and fixed a typo in a comment.
>>
>> Co-developed-by: Fox Chen <foxhlchen@gmail.com>
>> Signed-off-by: Fox Chen <foxhlchen@gmail.com>
>> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
>> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
>> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
>> Signed-off-by: Asahi Lina <lina@asahilina.net>
>> ---
>>  rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>
>> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
>> index cf3d089477d2..8a9222595cd1 100644
>> --- a/rust/kernel/error.rs
>> +++ b/rust/kernel/error.rs
>> @@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
>>      }
>>      Ok(ptr)
>>  }
>> +
>> +// TODO: Remove `dead_code` marker once an in-kernel client is available.
>> +#[allow(dead_code)]
>> +pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
>> +where
>> +    T: From<i16>,
>> +{
>> +    match r {
>> +        Ok(v) => v,
>> +        // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
>> +        // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
>> +        // therefore a negative `errno` always fits in an `i16` and will not overflow.
>> +        Err(e) => T::from(e.to_kernel_errno() as i16),
>> +    }
>> +}
>> +
>> +/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
>> +///
>> +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
>> +/// from inside `extern "C"` functions that need to return an integer
>> +/// error result.
>> +///
>> +/// `T` should be convertible from an `i16` via `From<i16>`.
>> +///
>> +/// # Examples
>> +///
>> +/// ```ignore
>> +/// # use kernel::from_kernel_result;
>> +/// # use kernel::bindings;
>> +/// unsafe extern "C" fn probe_callback(
>> +///     pdev: *mut bindings::platform_device,
>> +/// ) -> core::ffi::c_int {
>> +///     from_kernel_result! {
>> +///         let ptr = devm_alloc(pdev)?;
>> +///         bindings::platform_set_drvdata(pdev, ptr);
>> +///         Ok(0)
>> +///     }
>> +/// }
>> +/// ```
>> +// TODO: Remove `unused_macros` marker once an in-kernel client is available.
>> +#[allow(unused_macros)]
>> +macro_rules! from_kernel_result {
> 
> This actually doesn't need to be a macro, right? The following function
> version:
> 
> 	pub fn from_kernel_result<T, F>(f: F) -> T
> 	where
> 	    T: From<i16>,
> 	    F: FnOnce() -> Result<T>;
> 
> is not bad, the above case then can be written as:
> 
> 	unsafe extern "C" fn probe_callback(
> 	    pdev: *mut bindings::platform_device,
> 	) -> core::ffi::c_int {
> 	    from_kernel_result(|| {
> 		let ptr = devm_alloc(pdev)?;
> 		bindings::platform_set_drvdata(pdev, ptr);
> 		Ok(0)
> 	    })
> 	}
> 
> less magical, but the control flow is more clear.
> 
> Thoughts?

Looks good to me! I guess the macro was mostly to hide and call the
closure, but it's not really necessary. I'll change it ^^

~~ Lina