linux-kernel - Re: [PATCH 5/5] rust: error: Add from_kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 25 Feb 2023 11:31:13 +0900
From:   Asahi Lina <lina@...hilina.net>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Sven Van Asbroeck <thesven73@...il.com>,
        Fox Chen <foxhlchen@...il.com>, rust-for-linux@...r.kernel.org,
        linux-kernel@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH 5/5] rust: error: Add from_kernel_result!() macro

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@...il.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@...il.com>
>> Signed-off-by: Fox Chen <foxhlchen@...il.com>
>> Co-developed-by: Miguel Ojeda <ojeda@...nel.org>
>> Signed-off-by: Miguel Ojeda <ojeda@...nel.org>
>> Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
>> Signed-off-by: Asahi Lina <lina@...hilina.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