[PATCH v3 0/1] make `from_errno` use `try_from_errno`

[PATCH v3 0/1] make `from_errno` use `try_from_errno`

[Daniel Sedlak]

Currently, there are three patches for this issue. There was a race condition between
me [1] and Guilherme [2] because we both picked it up as a "good first issue" without
knowing that one of us had started working on it. We both agreed that one of us would
be the main author and the other one would be noted in "Codeveloped-by." However,
Guilherme has kept radio silence since [3]. So, I am resubmitting the patch, which
builds on top of [3]. 

Link: https://lore.kernel.org/rust-for-linux/20241104185135.18974-1-daniel@sedlak.dev/ [1]
Link: https://lore.kernel.org/rust-for-linux/20241104200014.12996-1-guilhermev2huehue@gmail.com/ [2]
Link: https://lore.kernel.org/rust-for-linux/20241105114819.14051-1-guilhermev2huehue@gmail.com/ [3]

Daniel Sedlak (1):
  rust: error: modify `from_errno` to use `try_from_errno`

 rust/kernel/error.rs | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

-- 
2.47.0

[PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Daniel Sedlak]

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

Link: https://github.com/Rust-for-Linux/linux/issues/1125
Suggested-by:  Miguel Ojeda <ojeda@kernel.org>
Co-developed-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/error.rs | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 52c502432447..236b3e4719e6 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -100,20 +100,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: crate::ffi::c_int) -> Error {
-        if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
+    pub fn from_errno(errno: core::ffi::c_int) -> Error {
+        if let Some(error) = Self::try_from_errno(errno) {
+            error
+        } else {
             // TODO: Make it a `WARN_ONCE` once available.
             crate::pr_warn!(
                 "attempted to create `Error` with out of range `errno`: {}",
                 errno
             );
-            return code::EINVAL;
+            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) }
     }
 
     /// Creates an [`Error`] from a kernel error code.
-- 
2.47.0

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Miguel Ojeda]

On Sat, Nov 23, 2024 at 11:49 AM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Modify the from_errno function to use try_from_errno to
> reduce code duplication while still maintaining all existing
> behavior and error handling and also reduces unsafe code.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1125
> Suggested-by:  Miguel Ojeda <ojeda@kernel.org>
> Co-developed-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
> Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>

Usually, when you have a cover letter, the title of the patch would
have been "[PATCH v3 1/1]", to indicate there was a cover letter. Did
`git format-patch` not generate it?

It is not important, of course, but I thought I would mention it in
case it helps.

> +    pub fn from_errno(errno: core::ffi::c_int) -> Error {
> +        if let Some(error) = Self::try_from_errno(errno) {
> +            error
> +        } else {

`let ... else` would work here, but given we just return the `error`
rather than having a long function afterwards, it is true it does not
really help.

Thanks for the patch to both Guilherme and you, and for navigating the
race condition on submitting it!

Cheers,
Miguel

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Daniel Sedlak]

On 11/23/24 2:45 PM, Miguel Ojeda wrote:
> On Sat, Nov 23, 2024 at 11:49 AM Daniel Sedlak <daniel@sedlak.dev> wrote:
>>
>> Modify the from_errno function to use try_from_errno to
>> reduce code duplication while still maintaining all existing
>> behavior and error handling and also reduces unsafe code.
>>
>> Link: https://github.com/Rust-for-Linux/linux/issues/1125
>> Suggested-by:  Miguel Ojeda <ojeda@kernel.org>
>> Co-developed-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
>> Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
>> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> 
> Usually, when you have a cover letter, the title of the patch would
> have been "[PATCH v3 1/1]", to indicate there was a cover letter. Did
> `git format-patch` not generate it?

Ops, I may used wrong series of commands, and this slipped away. Should 
I resend the patch with correct numbering?
> 
> It is not important, of course, but I thought I would mention it in
> case it helps.
> 
>> +    pub fn from_errno(errno: core::ffi::c_int) -> Error {
>> +        if let Some(error) = Self::try_from_errno(errno) {
>> +            error
>> +        } else {
> 
> `let ... else` would work here, but given we just return the `error`
> rather than having a long function afterwards, it is true it does not
> really help.

Thank you for the feedback and tip. Should I dig more into this and try 
to improve it more?
> 
> Thanks for the patch to both Guilherme and you, and for navigating the
> race condition on submitting it!
> 
> Cheers,
> Miguel

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Miguel Ojeda]

On Sat, Nov 23, 2024 at 8:08 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Ops, I may used wrong series of commands, and this slipped away. Should
> I resend the patch with correct numbering?
>
> Thank you for the feedback and tip. Should I dig more into this and try
> to improve it more?

No, no worries, it is all fine :)

Thanks!

Cheers,
Miguel

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Alice Ryhl]

On Sat, Nov 23, 2024 at 11:49 AM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Modify the from_errno function to use try_from_errno to
> reduce code duplication while still maintaining all existing
> behavior and error handling and also reduces unsafe code.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1125
> Suggested-by:  Miguel Ojeda <ojeda@kernel.org>
> Co-developed-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
> Signed-off-by: Guilherme Augusto Martins da Silva <guilhermev2huehue@gmail.com>
> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> ---
> -    pub fn from_errno(errno: crate::ffi::c_int) -> Error {
> +    pub fn from_errno(errno: core::ffi::c_int) -> Error {

This changes crate::ffi to core::ffi, but the correct thing is to use
crate::ffi.

Alice

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Miguel Ojeda]

On Mon, Nov 25, 2024 at 10:01 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> This changes crate::ffi to core::ffi, but the correct thing is to use
> crate::ffi.

Indeed, this seems to come from a bad rebase on top of the latest `rust-next`.

Cheers,
Miguel

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Daniel Sedlak]

On 11/25/24 10:27 AM, Miguel Ojeda wrote:
> On Mon, Nov 25, 2024 at 10:01 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> This changes crate::ffi to core::ffi, but the correct thing is to use
>> crate::ffi.
> 
> Indeed, this seems to come from a bad rebase on top of the latest `rust-next`.

You are right, I was developing this on top of `rust-next`. I get it 
that I should be developing on top of `rust-dev` right? If yes, should I 
also include the base commit id because of possible conflicts?
> 
> Cheers,
> Miguel

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Alice Ryhl]

On Mon, Nov 25, 2024 at 12:31 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
>
>
> On 11/25/24 10:27 AM, Miguel Ojeda wrote:
> > On Mon, Nov 25, 2024 at 10:01 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> This changes crate::ffi to core::ffi, but the correct thing is to use
> >> crate::ffi.
> >
> > Indeed, this seems to come from a bad rebase on top of the latest `rust-next`.
>
> You are right, I was developing this on top of `rust-next`. I get it
> that I should be developing on top of `rust-dev` right? If yes, should I
> also include the base commit id because of possible conflicts?

No. Developing on top of rust-next is preferable to rust-dev.


Alice

Re: [PATCH v3] rust: error: modify `from_errno` to use `try_from_errno`

[Miguel Ojeda]

On Mon, Nov 25, 2024 at 12:31 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> You are right, I was developing this on top of `rust-next`. I get it
> that I should be developing on top of `rust-dev` right? If yes, should I

What I meant is that it seemed like the line changed by `rust-next`
got overwritten when you resolved the conflict (since that line was
not a change in your original patch, just part of the context). In
other words, it seemed to me the mistake happened at rebase time, when
solving the conflict.

Using `rust-next` is perfectly fine and expected, as Alice mentioned.
You may also want to use other bases, like a mainline tag, as long as
you don't require the changes in `rust-next` (e.g. it is a fix that
will not go through that branch).

> also include the base commit id because of possible conflicts?

Please always include the base commit, it can save a lot of
time/confusion later on. Sometimes even when it is based on an
out-of-tree branch it is helpful (as long as it is published and
mentioned, of course). It can also spot submitting mistakes if nobody
knows what the hash is :)

Cheers,
Miguel

Re: [PATCH v3 0/1] make `from_errno` use `try_from_errno`

[Miguel Ojeda]

On Sat, Nov 23, 2024 at 11:49 AM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Currently, there are three patches for this issue. There was a race condition between
> me [1] and Guilherme [2] because we both picked it up as a "good first issue" without
> knowing that one of us had started working on it. We both agreed that one of us would
> be the main author and the other one would be noted in "Codeveloped-by." However,
> Guilherme has kept radio silence since [3]. So, I am resubmitting the patch, which
> builds on top of [3].

Sounds good, thanks!

(By the way, if you have a single patch, you can avoid a cover letter
to simplify by writing this right below the first `---` line in the
patch.)

Cheers,
Miguel