Feedback-ID: iad51458e:Fastmail
Date:   Fri, 24 Feb 2023 15:56:05 -0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Asahi Lina <lina@asahilina.net>
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Gary Guo <gary@garyguo.net>,
        =?iso-8859-1?Q?Bj=F6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
        Sven Van Asbroeck <thesven73@gmail.com>,
        Fox Chen <foxhlchen@gmail.com>, rust-for-linux@vger.kernel.org,
        linux-kernel@vger.kernel.org, asahi@lists.linux.dev
Subject: Re: [PATCH 5/5] rust: error: Add from_kernel_result!() macro
Message-ID: <Y/lOlcSpc+d9ytq/@boqun-archlinux>
References: <20230224-rust-error-v1-0-f8f9a9a87303@asahilina.net>
 <20230224-rust-error-v1-5-f8f9a9a87303@asahilina.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230224-rust-error-v1-5-f8f9a9a87303@asahilina.net>
Precedence: bulk

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
>