From: Jens Korinth <jens.korinth@tuta.io>

Use new pr_warn_once macro to resolve TODO in error.rs.

Signed-off-by: Jens Korinth <jens.korinth@tuta.io>
---
 rust/kernel/error.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 52c5024324474fc1306047f3fd7516f0023d0313..f6813dace1128b7ef91f64e79cd83bb64995bf97 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -102,8 +102,7 @@ impl Error {
     /// 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 {
-            // TODO: Make it a `WARN_ONCE` once available.
-            crate::pr_warn!(
+            crate::pr_warn_once!(
                 "attempted to create `Error` with out of range `errno`: {}",
                 errno
             );

-- 
2.47.0

On Tue, Nov 26, 2024 at 5:41 PM Jens Korinth via B4 Relay
<devnull+jens.korinth.tuta.io@kernel.org> wrote:
>
> Use new pr_warn_once macro to resolve TODO in error.rs.

Thanks for keeping the work on this!

I would mention here the merits between `pr_warn_once` vs. `WARN_ONCE`
and why the former was picked in this patch (especially since the
`TODO` suggests the latter).

Cheers,
Miguel

> I would mention here the merits between `pr_warn_once` vs. `WARN_ONCE`
> and why the former was picked in this patch (especially since the
> `TODO` suggests the latter).

Tbh, I am not 100% whether this should be here at all. The bug is not here, it's
at the call site. It should probably be a `try_from` instead, to raise the error
there?

Jens

On Wed, Nov 27, 2024 at 9:39 PM <jens.korinth@tuta.io> wrote:
>
> Tbh, I am not 100% whether this should be here at all. The bug is not here, it's
> at the call site. It should probably be a `try_from` instead, to raise the error
> there?

Do you mean removing the function altogether? i.e. migrating all
callers to `try_from_errno`?

Cheers,
Miguel

Sorry for the late response, the usual madness at the end of the year is
setting in.

> Do you mean removing the function altogether? i.e. migrating all
> callers to `try_from_errno`?

I think it should be `TryFrom`. The `std::From` doc [1] says:

    Note: This trait must not fail. The From trait is intended for perfect
    conversions. If the conversion can fail or is not perfect, use TryFrom.

[1]: https://doc.rust-lang.org/std/convert/trait.From.html

Jens

On Thu, Dec 5, 2024 at 7:30 AM <jens.korinth@tuta.io> wrote:
>
> Sorry for the late response, the usual madness at the end of the year is
> setting in.

No worries at all! I know the feeling... :)

> I think it should be `TryFrom`. The `std::From` doc [1] says:
>
>     Note: This trait must not fail. The From trait is intended for perfect
>     conversions. If the conversion can fail or is not perfect, use TryFrom.
>
> [1]: https://doc.rust-lang.org/std/convert/trait.From.html

Sorry, I am confused. This is not implementing the `From` trait, it is
an inherent implementation.

If what you mean is that this should not be an infallible operation,
then we are back at my previous reply, i.e. are you suggesting to
remove the method? Could you please clarify, perhaps with an example?

Or are you talking about something completely different?

Thanks!

Cheers,
Miguel

> Sorry, I am confused. This is not implementing the `From` trait, it is
> an inherent implementation.

Hmm, you're right. I assume there is a reason why it doesn't implement
`From<c_int>` or `TryFrom<c_int>`?

> If what you mean is that this should not be an infallible operation,
> then we are back at my previous reply, i.e. are you suggesting to
> remove the method? Could you please clarify, perhaps with an example?

Yes, I meant to say it should not be infallible, because, well, it isn't. :)
I'd probably make `try_from_errno` public and remove `from_errno`.

If there's no other disadvantage I'd remove `try_from_errno` as well and
replace it by `TryFrom<c_int>`. Implementing `From<Error> for c_int` may
also make sense for the other direction?

Jens