Hi everyone!
This series is part of the set of dependencies for the drm/asahi
Apple M1/M2 GPU driver.
It adds a bunch of missing wrappers in kernel::error, which are useful
to convert to/from kernel error codes. Since these will be used by many
abstractions coming up soon, I think it makes sense to merge them as
soon as possible instead of bundling them with the first user. Hence,
they have allow() tags to silence dead code warnings. These can be
removed as soon as the first user is in the kernel crate.
Getting this in first allows the subsequent abstractions to be merged in
any order, so we don't have to worry about piecewise rebasing and fixing
conflicts in the Error wrappers. See [1] for a complete tree with the DRM
abstractions and all other miscellaneous work-in-progress prerequisites
rebased on top of mainline.
Most of these have been extracted from the rust-for-linux/rust branch,
with author attribution to the first/primary author and Co-developed-by:
for everyone else who touched the code.
Attribution changes:
- One of the patches had Miguel's old email in the tags, updated that per
his request.
- Wedson's email changed from @google.com to @gmail.com (I understand
this is the current one).
Sven: There is one patch from you in this series, do you want to send it
yourself directly? I understand Wedson and Miguel are okay with me
sending stuff on their behalf.
[1] https://github.com/Rust-for-Linux/linux/pull/969/commits
Signed-off-by: Asahi Lina <[email protected]>
---
Asahi Lina (1):
rust: error: Add Error::to_ptr()
Miguel Ojeda (1):
rust: error: Add Error::from_kernel_errno()
Sven Van Asbroeck (1):
rust: error: Add a helper to convert a C ERR_PTR to a `Result`
Wedson Almeida Filho (2):
rust: error: Add to_result() helper
rust: error: Add from_kernel_result!() macro
rust/helpers.c | 19 +++++++
rust/kernel/error.rs | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 156 insertions(+)
---
base-commit: 83f978b63fa7ad474ca22d7e2772c5988101c9bd
change-id: 20230224-rust-error-f2871fbc907f
Thank you,
~~ Lina
This is the Rust equivalent to ERR_PTR(), for use in C callbacks.
Marked as #[allow(dead_code)] for now, since it does not have any
consumers yet.
Signed-off-by: Asahi Lina <[email protected]>
---
rust/helpers.c | 7 +++++++
rust/kernel/error.rs | 7 +++++++
2 files changed, 14 insertions(+)
diff --git a/rust/helpers.c b/rust/helpers.c
index 09a4d93f9d62..89f4cd1e0df3 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -20,6 +20,7 @@
#include <linux/bug.h>
#include <linux/build_bug.h>
+#include <linux/err.h>
#include <linux/refcount.h>
__noreturn void rust_helper_BUG(void)
@@ -46,6 +47,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
}
EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
+__force void *rust_helper_ERR_PTR(long err)
+{
+ return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
+
/*
* We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
* as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 5b9751d7ff1d..8611758e27f4 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -76,6 +76,13 @@ impl Error {
pub fn to_kernel_errno(self) -> core::ffi::c_int {
self.0
}
+
+ /// Returns the error encoded as a pointer.
+ #[allow(dead_code)]
+ pub(crate) fn to_ptr<T>(self) -> *mut T {
+ // SAFETY: Valid as long as self.0 is a valid error
+ unsafe { bindings::ERR_PTR(self.0.into()) as *mut _ }
+ }
}
impl From<AllocError> for Error {
--
2.35.1
From: Miguel Ojeda <[email protected]>
Add a function to create `Error` values out of a kernel error return,
which safely upholds the invariant that the error code is well-formed
(negative and greater than -MAX_ERRNO). If a malformed code is passed
in, it will be converted to EINVAL.
Lina: Imported from rust-for-linux/rust as authored by Miguel and Fox
with refactoring from Wedson.
Co-developed-by: Fox Chen <[email protected]>
Signed-off-by: Fox Chen <[email protected]>
Co-developed-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
rust/kernel/error.rs | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 8611758e27f4..3b439fdb405c 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -72,6 +72,25 @@ pub mod code {
pub struct Error(core::ffi::c_int);
impl Error {
+ /// Creates an [`Error`] from a kernel error code.
+ ///
+ /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
+ /// be returned in such a case.
+ pub(crate) fn from_kernel_errno(errno: core::ffi::c_int) -> Error {
+ if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
+ // TODO: Make it a `WARN_ONCE` once available.
+ crate::pr_warn!(
+ "attempted to create `Error` with out of range `errno`: {}",
+ errno
+ );
+ return code::EINVAL;
+ }
+
+ // INVARIANT: The check above ensures the type invariant
+ // will hold.
+ Error(errno)
+ }
+
/// Returns the kernel error code.
pub fn to_kernel_errno(self) -> core::ffi::c_int {
self.0
--
2.35.1
From: Wedson Almeida Filho <[email protected]>
Add a to_result() helper to convert kernel C return values to a Rust
Result, mapping >=0 values to Ok(()) and negative values to Err(...),
with Error::from_kernel_errno() ensuring that the errno is within range.
Lina: Imported from rust-for-linux/rust, originally developed by Wedson
as part of the AMBA device driver support.
Signed-off-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
rust/kernel/error.rs | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 3b439fdb405c..1e8371f28746 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -167,3 +167,13 @@ impl From<core::convert::Infallible> for Error {
/// it should still be modeled as returning a `Result` rather than
/// just an [`Error`].
pub type Result<T = ()> = core::result::Result<T, Error>;
+
+/// 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: core::ffi::c_int) -> Result {
+ if err < 0 {
+ Err(Error::from_kernel_errno(err))
+ } else {
+ Ok(())
+ }
+}
--
2.35.1
From: Sven Van Asbroeck <[email protected]>
Some kernel C API functions return a pointer which embeds an optional
`errno`. Callers are supposed to check the returned pointer with
`IS_ERR()` and if this returns `true`, retrieve the `errno` using
`PTR_ERR()`.
Create a Rust helper function to implement the Rust equivalent:
transform a `*mut T` to `Result<*mut T>`.
Lina: Imported from rust-for-linux/linux, with subsequent refactoring
and contributions squashed in and attributed below. Replaced usage of
from_kernel_errno_unchecked() with an open-coded constructor, since this
is the only user anyway.
Co-developed-by: Boqun Feng <[email protected]>
Signed-off-by: Boqun Feng <[email protected]>
Co-developed-by: Miguel Ojeda <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Co-developed-by: Fox Chen <[email protected]>
Signed-off-by: Fox Chen <[email protected]>
Co-developed-by: Gary Guo <[email protected]>
Signed-off-by: Gary Guo <[email protected]>
Signed-off-by: Sven Van Asbroeck <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
rust/helpers.c | 12 ++++++++++++
rust/kernel/error.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/rust/helpers.c b/rust/helpers.c
index 89f4cd1e0df3..04b9be46e887 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -53,6 +53,18 @@ __force void *rust_helper_ERR_PTR(long err)
}
EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
+bool rust_helper_IS_ERR(__force const void *ptr)
+{
+ return IS_ERR(ptr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_IS_ERR);
+
+long rust_helper_PTR_ERR(__force const void *ptr)
+{
+ return PTR_ERR(ptr);
+}
+EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
+
/*
* We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
* as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 1e8371f28746..cf3d089477d2 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -177,3 +177,52 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
Ok(())
}
}
+
+/// Transform a kernel "error pointer" to a normal pointer.
+///
+/// Some kernel C API functions return an "error pointer" which optionally
+/// embeds an `errno`. Callers are supposed to check the returned pointer
+/// for errors. This function performs the check and converts the "error pointer"
+/// to a normal pointer in an idiomatic fashion.
+///
+/// # Examples
+///
+/// ```ignore
+/// # use kernel::from_kernel_err_ptr;
+/// # use kernel::bindings;
+/// fn devm_platform_ioremap_resource(
+/// pdev: &mut PlatformDevice,
+/// index: u32,
+/// ) -> Result<*mut core::ffi::c_void> {
+/// // SAFETY: FFI call.
+/// unsafe {
+/// from_kernel_err_ptr(bindings::devm_platform_ioremap_resource(
+/// pdev.to_ptr(),
+/// index,
+/// ))
+/// }
+/// }
+/// ```
+// TODO: Remove `dead_code` marker once an in-kernel client is available.
+#[allow(dead_code)]
+pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
+ // CAST: Casting a pointer to `*const core::ffi::c_void` is always valid.
+ let const_ptr: *const core::ffi::c_void = ptr.cast();
+ // SAFETY: The FFI function does not deref the pointer.
+ if unsafe { bindings::IS_ERR(const_ptr) } {
+ // SAFETY: The FFI function does not deref the pointer.
+ let err = unsafe { bindings::PTR_ERR(const_ptr) };
+ // CAST: If `IS_ERR()` returns `true`,
+ // then `PTR_ERR()` is guaranteed to return a
+ // negative value greater-or-equal to `-bindings::MAX_ERRNO`,
+ // which always fits in an `i16`, as per the invariant above.
+ // And an `i16` always fits in an `i32`. So casting `err` to
+ // an `i32` can never overflow, and is always valid.
+ //
+ // SAFETY: `IS_ERR()` ensures `err` is a
+ // negative value greater-or-equal to `-bindings::MAX_ERRNO`.
+ #[cfg_attr(CONFIG_ARM, allow(clippy::unnecessary_cast))]
+ return Err(Error(err as i32));
+ }
+ Ok(ptr)
+}
--
2.35.1
From: Wedson Almeida Filho <[email protected]>
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 <[email protected]>
Signed-off-by: Fox Chen <[email protected]>
Co-developed-by: Miguel Ojeda <[email protected]>
Signed-off-by: Miguel Ojeda <[email protected]>
Signed-off-by: Wedson Almeida Filho <[email protected]>
Signed-off-by: Asahi Lina <[email protected]>
---
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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Fox Chen <[email protected]>
> Co-developed-by: Miguel Ojeda <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
> Signed-off-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
> 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 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Fox Chen <[email protected]>
>> Co-developed-by: Miguel Ojeda <[email protected]>
>> Signed-off-by: Miguel Ojeda <[email protected]>
>> Signed-off-by: Wedson Almeida Filho <[email protected]>
>> Signed-off-by: Asahi Lina <[email protected]>
>> ---
>> 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
On Fri, 24 Feb 2023 17:50:19 +0900
Asahi Lina <[email protected]> wrote:
> This is the Rust equivalent to ERR_PTR(), for use in C callbacks.
> Marked as #[allow(dead_code)] for now, since it does not have any
> consumers yet.
>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
> rust/helpers.c | 7 +++++++
> rust/kernel/error.rs | 7 +++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 09a4d93f9d62..89f4cd1e0df3 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -20,6 +20,7 @@
>
> #include <linux/bug.h>
> #include <linux/build_bug.h>
> +#include <linux/err.h>
> #include <linux/refcount.h>
>
> __noreturn void rust_helper_BUG(void)
> @@ -46,6 +47,12 @@ bool rust_helper_refcount_dec_and_test(refcount_t *r)
> }
> EXPORT_SYMBOL_GPL(rust_helper_refcount_dec_and_test);
>
> +__force void *rust_helper_ERR_PTR(long err)
> +{
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
> +
I know that we already have `IS_ERR` in helpers.c, but having to go
through FFI and helper functions for something as simple as a cast
feels awkward to me.
Given that `ERR_PTR`'s C definition is very unlike to change, would it
be problematic if we just reimplement it in Rust as
```rust
fn ERR_PTR(error: core::ffi::c_long) -> *mut core::ffi::c_void {
error as _
// Or `core::ptr::invalid(error as _)` with strict provenance
}
```
?
I personally think it should be fine, but I'll leave the decision to
Miguel.
Best,
Gary
On Fri, 24 Feb 2023 17:50:20 +0900
Asahi Lina <[email protected]> wrote:
> From: Miguel Ojeda <[email protected]>
>
> Add a function to create `Error` values out of a kernel error return,
> which safely upholds the invariant that the error code is well-formed
> (negative and greater than -MAX_ERRNO). If a malformed code is passed
> in, it will be converted to EINVAL.
>
> Lina: Imported from rust-for-linux/rust as authored by Miguel and Fox
> with refactoring from Wedson.
>
> Co-developed-by: Fox Chen <[email protected]>
> Signed-off-by: Fox Chen <[email protected]>
> Co-developed-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
> rust/kernel/error.rs | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 8611758e27f4..3b439fdb405c 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -72,6 +72,25 @@ pub mod code {
> pub struct Error(core::ffi::c_int);
>
> impl Error {
> + /// Creates an [`Error`] from a kernel error code.
> + ///
> + /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
> + /// be returned in such a case.
> + pub(crate) fn from_kernel_errno(errno: core::ffi::c_int) -> Error {
Maybe just `from_errno`? I don't know why `kernel` would need to be
emphasised here.
Best,
Gary
On Fri, 24 Feb 2023 15:56:05 -0800
Boqun Feng <[email protected]> wrote:
> On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> > From: Wedson Almeida Filho <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Fox Chen <[email protected]>
> > Co-developed-by: Miguel Ojeda <[email protected]>
> > Signed-off-by: Miguel Ojeda <[email protected]>
> > Signed-off-by: Wedson Almeida Filho <[email protected]>
> > Signed-off-by: Asahi Lina <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> > On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> > > From: Wedson Almeida Filho <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Fox Chen <[email protected]>
> > > Co-developed-by: Miguel Ojeda <[email protected]>
> > > Signed-off-by: Miguel Ojeda <[email protected]>
> > > Signed-off-by: Wedson Almeida Filho <[email protected]>
> > > Signed-off-by: Asahi Lina <[email protected]>
> > > ---
> > > 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 Sat, 25 Feb 2023 18:22:11 -0800
Boqun Feng <[email protected]> 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 <[email protected]> wrote:
> >
> > > On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> > > > From: Wedson Almeida Filho <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Fox Chen <[email protected]>
> > > > Co-developed-by: Miguel Ojeda <[email protected]>
> > > > Signed-off-by: Miguel Ojeda <[email protected]>
> > > > Signed-off-by: Wedson Almeida Filho <[email protected]>
> > > > Signed-off-by: Asahi Lina <[email protected]>
> > > > ---
> > > > 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 3:22 AM Boqun Feng <[email protected]> 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, Feb 25, 2023 at 11:19 PM Gary Guo <[email protected]> wrote:
>
> Maybe just `from_errno`? I don't know why `kernel` would need to be
> emphasised here.
Yeah, that sounds good to me. It is clear and we don't use "errno"
elsewhere. This identifier came from the original import, so before we
started to think about naming policies.
Though perhaps we can clean it up in a patch later, since we should
change `to_kernel_errno` below too at the same time. Or if you want to
send a quick patch for that one, I can put it in first.
Cheers,
Miguel
On Sat, Feb 25, 2023 at 11:14 PM Gary Guo <[email protected]> wrote:
>
> I know that we already have `IS_ERR` in helpers.c, but having to go
> through FFI and helper functions for something as simple as a cast
> feels awkward to me.
>
> Given that `ERR_PTR`'s C definition is very unlike to change, would it
> be problematic if we just reimplement it in Rust as
>
> ```rust
> fn ERR_PTR(error: core::ffi::c_long) -> *mut core::ffi::c_void {
> error as _
> // Or `core::ptr::invalid(error as _)` with strict provenance
> }
> ```
> ?
>
> I personally think it should be fine, but I'll leave the decision to
> Miguel.
On one hand, we have tried to minimize duplication (and, in general,
any changes to the C side) so far where possible, especially
pre-merge, doing it only when needed, e.g. for `const` purposes.
On the other hand, being in the kernel opens up a few possibilities to
consider, and it is true it feels like some of these could get
reimplemented, even if not strictly needed. If we can show a
performance/text size difference on e.g. a non-trivial subsystem or
module, I think we should do it.
If we do it, then I think we should add a note on the C side so that
it is clear there is a duplicated implementation elsewhere, avoiding
future problems. In fact, it would be ideal to do it consistently,
e.g. also for the ioctl ones. Something like:
/* Rust: reimplemented as `kernel::error::ERR_PTR`. */
static inline void * __must_check ERR_PTR(long error)
{
return (void *) error;
}
Or perhaps something even smaller.
But I don't want to block the rest of the work on this, which may need
some extra/parallel discussion, so let's keep the helper for the time
being. That way we can also do that change independently and justify
the change showing the difference in performance/text, if any, in the
commit message.
Cheers,
Miguel
On Sun, Feb 26, 2023 at 01:36:06PM +0000, Gary Guo wrote:
> On Sat, 25 Feb 2023 18:22:11 -0800
> Boqun Feng <[email protected]> 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 <[email protected]> wrote:
> > >
> > > > On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> > > > > From: Wedson Almeida Filho <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > Signed-off-by: Fox Chen <[email protected]>
> > > > > Co-developed-by: Miguel Ojeda <[email protected]>
> > > > > Signed-off-by: Miguel Ojeda <[email protected]>
> > > > > Signed-off-by: Wedson Almeida Filho <[email protected]>
> > > > > Signed-off-by: Asahi Lina <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]> 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 Sun, Feb 26, 2023 at 09:59:25PM +0100, Miguel Ojeda wrote:
> On Sun, Feb 26, 2023 at 7:17 PM Boqun Feng <[email protected]> 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 <[email protected]> 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
Asahi Lina <[email protected]> writes:
> From: Wedson Almeida Filho <[email protected]>
>
> Add a to_result() helper to convert kernel C return values to a Rust
> Result, mapping >=0 values to Ok(()) and negative values to Err(...),
> with Error::from_kernel_errno() ensuring that the errno is within range.
>
> Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> as part of the AMBA device driver support.
>
> Signed-off-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
Reviewed-by: Andreas Hindborg <[email protected]>
> rust/kernel/error.rs | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 3b439fdb405c..1e8371f28746 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -167,3 +167,13 @@ impl From<core::convert::Infallible> for Error {
> /// it should still be modeled as returning a `Result` rather than
> /// just an [`Error`].
> pub type Result<T = ()> = core::result::Result<T, Error>;
> +
> +/// 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: core::ffi::c_int) -> Result {
> + if err < 0 {
> + Err(Error::from_kernel_errno(err))
> + } else {
> + Ok(())
> + }
> +}
Asahi Lina <[email protected]> writes:
> From: Miguel Ojeda <[email protected]>
>
> Add a function to create `Error` values out of a kernel error return,
> which safely upholds the invariant that the error code is well-formed
> (negative and greater than -MAX_ERRNO). If a malformed code is passed
> in, it will be converted to EINVAL.
>
> Lina: Imported from rust-for-linux/rust as authored by Miguel and Fox
> with refactoring from Wedson.
>
> Co-developed-by: Fox Chen <[email protected]>
> Signed-off-by: Fox Chen <[email protected]>
> Co-developed-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Wedson Almeida Filho <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
Reviewed-by: Andreas Hindborg <[email protected]>
> rust/kernel/error.rs | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 8611758e27f4..3b439fdb405c 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -72,6 +72,25 @@ pub mod code {
> pub struct Error(core::ffi::c_int);
>
> impl Error {
> + /// Creates an [`Error`] from a kernel error code.
> + ///
> + /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
> + /// be returned in such a case.
> + pub(crate) fn from_kernel_errno(errno: core::ffi::c_int) -> Error {
> + if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
> + // TODO: Make it a `WARN_ONCE` once available.
> + crate::pr_warn!(
> + "attempted to create `Error` with out of range `errno`: {}",
> + errno
> + );
> + return code::EINVAL;
> + }
> +
> + // INVARIANT: The check above ensures the type invariant
> + // will hold.
> + Error(errno)
> + }
> +
> /// Returns the kernel error code.
> pub fn to_kernel_errno(self) -> core::ffi::c_int {
> self.0
Asahi Lina <[email protected]> writes:
> From: Sven Van Asbroeck <[email protected]>
>
> Some kernel C API functions return a pointer which embeds an optional
> `errno`. Callers are supposed to check the returned pointer with
> `IS_ERR()` and if this returns `true`, retrieve the `errno` using
> `PTR_ERR()`.
>
> Create a Rust helper function to implement the Rust equivalent:
> transform a `*mut T` to `Result<*mut T>`.
>
> Lina: Imported from rust-for-linux/linux, with subsequent refactoring
> and contributions squashed in and attributed below. Replaced usage of
> from_kernel_errno_unchecked() with an open-coded constructor, since this
> is the only user anyway.
>
> Co-developed-by: Boqun Feng <[email protected]>
> Signed-off-by: Boqun Feng <[email protected]>
> Co-developed-by: Miguel Ojeda <[email protected]>
> Signed-off-by: Miguel Ojeda <[email protected]>
> Co-developed-by: Fox Chen <[email protected]>
> Signed-off-by: Fox Chen <[email protected]>
> Co-developed-by: Gary Guo <[email protected]>
> Signed-off-by: Gary Guo <[email protected]>
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> Signed-off-by: Asahi Lina <[email protected]>
> ---
> rust/helpers.c | 12 ++++++++++++
> rust/kernel/error.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 89f4cd1e0df3..04b9be46e887 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -53,6 +53,18 @@ __force void *rust_helper_ERR_PTR(long err)
> }
> EXPORT_SYMBOL_GPL(rust_helper_ERR_PTR);
>
> +bool rust_helper_IS_ERR(__force const void *ptr)
> +{
> + return IS_ERR(ptr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_IS_ERR);
> +
> +long rust_helper_PTR_ERR(__force const void *ptr)
> +{
> + return PTR_ERR(ptr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_PTR_ERR);
> +
> /*
> * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
> * as the Rust `usize` type, so we can use it in contexts where Rust
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 1e8371f28746..cf3d089477d2 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -177,3 +177,52 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
> Ok(())
> }
> }
> +
> +/// Transform a kernel "error pointer" to a normal pointer.
> +///
> +/// Some kernel C API functions return an "error pointer" which optionally
> +/// embeds an `errno`. Callers are supposed to check the returned pointer
> +/// for errors. This function performs the check and converts the "error pointer"
> +/// to a normal pointer in an idiomatic fashion.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// # use kernel::from_kernel_err_ptr;
> +/// # use kernel::bindings;
> +/// fn devm_platform_ioremap_resource(
> +/// pdev: &mut PlatformDevice,
> +/// index: u32,
> +/// ) -> Result<*mut core::ffi::c_void> {
> +/// // SAFETY: FFI call.
> +/// unsafe {
> +/// from_kernel_err_ptr(bindings::devm_platform_ioremap_resource(
> +/// pdev.to_ptr(),
> +/// index,
> +/// ))
> +/// }
> +/// }
> +/// ```
> +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> +#[allow(dead_code)]
> +pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
For consistency, if `from_kernel_errno()` should be `from_errno()` this
should be `from_err_ptr()` as well?
BR Andreas
> + // CAST: Casting a pointer to `*const core::ffi::c_void` is always valid.
> + let const_ptr: *const core::ffi::c_void = ptr.cast();
> + // SAFETY: The FFI function does not deref the pointer.
> + if unsafe { bindings::IS_ERR(const_ptr) } {
> + // SAFETY: The FFI function does not deref the pointer.
> + let err = unsafe { bindings::PTR_ERR(const_ptr) };
> + // CAST: If `IS_ERR()` returns `true`,
> + // then `PTR_ERR()` is guaranteed to return a
> + // negative value greater-or-equal to `-bindings::MAX_ERRNO`,
> + // which always fits in an `i16`, as per the invariant above.
> + // And an `i16` always fits in an `i32`. So casting `err` to
> + // an `i32` can never overflow, and is always valid.
> + //
> + // SAFETY: `IS_ERR()` ensures `err` is a
> + // negative value greater-or-equal to `-bindings::MAX_ERRNO`.
> + #[cfg_attr(CONFIG_ARM, allow(clippy::unnecessary_cast))]
> + return Err(Error(err as i32));
> + }
> + Ok(ptr)
> +}
On Mon, Feb 27, 2023 at 2:43 PM Andreas Hindborg <[email protected]> wrote:
>
> For consistency, if `from_kernel_errno()` should be `from_errno()` this
> should be `from_err_ptr()` as well?
Sounds good to me.
Cheers,
Miguel
On 2/26/23 17:59, Miguel Ojeda wrote:
> On Sun, Feb 26, 2023 at 7:17 PM Boqun Feng <[email protected]> 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 Mon, Feb 27, 2023 at 01:10:39PM +0100, Miguel Ojeda wrote:
> On Sun, Feb 26, 2023 at 11:13 PM Boqun Feng <[email protected]> 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 Sun, Feb 26, 2023 at 3:26 PM Miguel Ojeda
<[email protected]> wrote:
>
> But I don't want to block the rest of the work on this, which may need
> some extra/parallel discussion, so let's keep the helper for the time
> being. That way we can also do that change independently and justify
> the change showing the difference in performance/text, if any, in the
> commit message.
Opened https://github.com/Rust-for-Linux/linux/issues/984 to help to
remember it.
Cheers,
Miguel