[PATCH] rust-next: make from_errno use try_from_errno

[PATCH] rust-next: make from_errno use try_from_errno

[Guilherme Augusto Martins da Silva]

Make from_errno use try_from_errno (available in rust-next).  

This also allows to remove unsafe code and a comment or two too.
 this project or the open source license(s) involved.

Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
---
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 7cd3bbab52f2..077f95cbb6ae 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -101,19 +101,17 @@ impl Error {
     /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
     /// be returned in such a case.
     pub fn from_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;
+        match Error::try_from_errno(errno) {
+            Some(error) => error,
+            None => {
+                // TODO: Make it a `WARN_ONCE` once available.
+                crate::pr_warn!(
+                    "attempted to create `Error` with out of range `errno`: {}",
+                    errno
+                );
+                code::EINVAL
+            }
         }
-
-        // INVARIANT: The check above ensures the type invariant
-        // will hold.
-        // SAFETY: `errno` is checked above to be in a valid range.
-        unsafe { Error::from_errno_unchecked(errno) }
     }

Re: [PATCH] rust-next: make from_errno use try_from_errno

[Miguel Ojeda]

Hi Guilherme, Daniel,

On Mon, Nov 4, 2024 at 9:01 PM Guilherme Augusto Martins da Silva
<guilhermev2huehue@gmail.com> wrote:
>
> Make from_errno use try_from_errno (available in rust-next).

Normally you would note the "available in rust-next" part in the
non-commit part (i.e. below the `---` line).

>  this project or the open source license(s) involved.

Spurious line from the previous Developer's Certificate of Origin you
had in the commit message (in private).

> Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>

Please add the Suggested-by and the Link tags as mentioned in the issue.

So I got these two at essentially the same time, but Guilherme's in
private, as we discussed.

I think you should both be co-authors, although you will need to
decide who will be the main author and send a v2 merging both patches
into the best version possible :)

Please see my notes in the other one too.

Cheers,
Miguel

[PATCH] rust-next: make from_errno use try_from_errno

[Guilherme Augusto Martins da Silva]

Modified the from_errno function to use try_from_errno to reduce code duplication while still maintaning all existing behavior and error handling and also reduces unsafe code.

Co-developed-by: Daniel Sedlak <daniel@sedlak.dev>
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
Suggested-by:  Miguel Ojeda <ojeda@kernel.org>
Link: https://github.com/Rust-for-Linux/linux/issues/1125
---
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 7cd3bbab52f2..077f95cbb6ae 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -101,19 +101,17 @@ impl Error {
     /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
     /// be returned in such a case.
     pub fn from_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;
+        match Error::try_from_errno(errno) {
+            Some(error) => error,
+            None => {
+                // TODO: Make it a `WARN_ONCE` once available.
+                crate::pr_warn!(
+                    "attempted to create `Error` with out of range `errno`: {}",
+                    errno
+                );
+                code::EINVAL
+            }
         }
-
-        // INVARIANT: The check above ensures the type invariant
-        // will hold.
-        // SAFETY: `errno` is checked above to be in a valid range.
-        unsafe { Error::from_errno_unchecked(errno) }
     }

Re: [PATCH] rust-next: make from_errno use try_from_errno

[Rasmus Villemoes]

On Tue, Nov 05 2024, Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com> wrote:

> Modified the from_errno function to use try_from_errno to reduce code duplication while still maintaning all existing behavior and error handling and also reduces unsafe code.
>
> Co-developed-by: Daniel Sedlak <daniel@sedlak.dev>
> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
> Suggested-by:  Miguel Ojeda <ojeda@kernel.org>
> Link: https://github.com/Rust-for-Linux/linux/issues/1125
> ---
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 7cd3bbab52f2..077f95cbb6ae 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -101,19 +101,17 @@ impl Error {
>      /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
>      /// be returned in such a case.
>      pub fn from_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;
> +        match Error::try_from_errno(errno) {
> +            Some(error) => error,
> +            None => {
> +                // TODO: Make it a `WARN_ONCE` once available.
> +                crate::pr_warn!(
> +                    "attempted to create `Error` with out of range `errno`: {}",
> +                    errno
> +                );
> +                code::EINVAL
> +            }


Not really related to the change done here, but I've often wondered why
the kernel doesn't have a dedicated EBUG that can be used in places
where a genuine kernel bug is detected at run-time, but BUG() is too
much, and we can survive by just passing some error back up the stack
(possibly with a WARN or pr_warn()). In fact, an EBUG could be useful in
userspace code as well for library code that detects some
should-not-happen inconsistency.

EINVAL is already way overused, and substituting that when a programming
error rather than bad user input is detected seems to just overload it
even more.

Rasmus

Re: [PATCH] rust-next: make from_errno use try_from_errno

[Daniel Sedlak]

Hello Guilherme,
I believe you need to send a new patch as a `v2` instead of pushing
changes to this patch.
Also in your commit message, I believe you should only write `rust:`
instead of `rust-next:`
see other patches in the mailing list.

On 11/5/24 12:48 PM, Guilherme Augusto Martins da Silva wrote:

Modified the from_errno function to use try_from_errno to reduce code
duplication while still maintaning all existing behavior and error
handling and also reduces unsafe code.

Co-developed-by: Daniel Sedlak <daniel@sedlak.dev>
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
Suggested-by:  Miguel Ojeda <ojeda@kernel.org>
Link: https://github.com/Rust-for-Linux/linux/issues/1125
---
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 7cd3bbab52f2..077f95cbb6ae 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -101,19 +101,17 @@ impl Error {
     /// It is a bug to pass an out-of-range `errno`. `EINVAL` would
     /// be returned in such a case.
     pub fn from_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;
+        match Error::try_from_errno(errno) {
+            Some(error) => error,
+            None => {
+                // TODO: Make it a `WARN_ONCE` once available.
+                crate::pr_warn!(
+                    "attempted to create `Error` with out of range
`errno`: {}",
+                    errno
+                );
+                code::EINVAL
+            }
         }

This is highly opinionated so do what satisfy you, but I believe the
`if let … else …` pattern is better,
because of the indentation and less lines changed, see my previous
patch: https://lore.kernel.org/rust-for-linux/20241104185135.18974-1-daniel@sedlak.dev/.

-
-        // INVARIANT: The check above ensures the type invariant
-        // will hold.
-        // SAFETY: `errno` is checked above to be in a valid range.
-        unsafe { Error::from_errno_unchecked(errno) }
     }