[PATCH] [RFC] rust: error: Convert 0 being an error to Result

[PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Dirk Behme]

The existing to_result() takes a (signed) integer from a kernel C function
and converts it to an error if it's negative. Additionally, there are
kernel C functions returning an unsigned integer where 0 is the error case.
For example gen_pool_alloc() and friends. Provide a mechanism to convert
this to Result too.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 rust/kernel/error.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

I'm unsure if something like this is acceptable. Therefore the RFC.
But I want at least ask ;) In the end this is a slightly modified
copy of the existing to_result().

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 376280b6a745a..3306b8f590866 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -248,6 +248,16 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
     }
 }
 
+/// Converts an unsigned integer as returned by a C kernel function to EINVAL if it's zero,
+/// and `Ok(u64)` otherwise.
+pub fn to_result_zero(val: core::ffi::c_ulong) -> Result<u64> {
+    if val == 0 {
+        Err(code::EINVAL)
+    } else {
+        Ok(val)
+    }
+}
+
 /// Transform a kernel "error pointer" to a normal pointer.
 ///
 /// Some kernel C API functions return an "error pointer" which optionally
-- 
2.28.0

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Greg KH]

On Thu, Jan 11, 2024 at 07:44:15AM +0100, Dirk Behme wrote:
> The existing to_result() takes a (signed) integer from a kernel C function
> and converts it to an error if it's negative. Additionally, there are
> kernel C functions returning an unsigned integer where 0 is the error case.
> For example gen_pool_alloc() and friends. Provide a mechanism to convert
> this to Result too.
> 
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  rust/kernel/error.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> I'm unsure if something like this is acceptable. Therefore the RFC.
> But I want at least ask ;) In the end this is a slightly modified
> copy of the existing to_result().
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 376280b6a745a..3306b8f590866 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -248,6 +248,16 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
>      }
>  }
>  
> +/// Converts an unsigned integer as returned by a C kernel function to EINVAL if it's zero,
> +/// and `Ok(u64)` otherwise.
> +pub fn to_result_zero(val: core::ffi::c_ulong) -> Result<u64> {
> +    if val == 0 {
> +        Err(code::EINVAL)
> +    } else {
> +        Ok(val)
> +    }
> +}

How would this be used?  0 is normally not an error, why would you
need/want to turn that into an error value?

thanks,

greg k-h

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Thu, 11 Jan 2024 07:44:15 +0100
Dirk Behme <dirk.behme@de.bosch.com> wrote:

> +/// Converts an unsigned integer as returned by a C kernel function to EINVAL if it's zero,
> +/// and `Ok(u64)` otherwise.
> +pub fn to_result_zero(val: core::ffi::c_ulong) -> Result<u64> {
> +    if val == 0 {
> +        Err(code::EINVAL)
> +    } else {
> +        Ok(val)
> +    }
> +}

Would it make sense to use Result<NonZeroU64> as a return type and make the function inline?

-- 
Michael Büsch
https://bues.ch/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Behme Dirk (CM/ESO2)]

On 11.01.2024 07:59, Greg KH wrote:
> On Thu, Jan 11, 2024 at 07:44:15AM +0100, Dirk Behme wrote:
>> The existing to_result() takes a (signed) integer from a kernel C function
>> and converts it to an error if it's negative. Additionally, there are
>> kernel C functions returning an unsigned integer where 0 is the error case.
>> For example gen_pool_alloc() and friends. Provide a mechanism to convert
>> this to Result too.
>>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>>   rust/kernel/error.rs | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> I'm unsure if something like this is acceptable. Therefore the RFC.
>> But I want at least ask ;) In the end this is a slightly modified
>> copy of the existing to_result().
>>
>> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
>> index 376280b6a745a..3306b8f590866 100644
>> --- a/rust/kernel/error.rs
>> +++ b/rust/kernel/error.rs
>> @@ -248,6 +248,16 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
>>       }
>>   }
>>   
>> +/// Converts an unsigned integer as returned by a C kernel function to EINVAL if it's zero,
>> +/// and `Ok(u64)` otherwise.
>> +pub fn to_result_zero(val: core::ffi::c_ulong) -> Result<u64> {
>> +    if val == 0 {
>> +        Err(code::EINVAL)
>> +    } else {
>> +        Ok(val)
>> +    }
>> +}
> 
> How would this be used? 

One (randomly selected) usage example:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/cpu/mce/genpool.c#n107

node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
if (!node) {
	pr_warn_ratelimited("MCE records pool full!\n");
	return -ENOMEM;
}

> 0 is normally not an error, why would you
> need/want to turn that into an error value?

Best regards

Dirk

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Greg KH]

On Thu, Jan 11, 2024 at 08:29:06AM +0100, Behme Dirk (CM/ESO2) wrote:
> On 11.01.2024 07:59, Greg KH wrote:
> > On Thu, Jan 11, 2024 at 07:44:15AM +0100, Dirk Behme wrote:
> > > The existing to_result() takes a (signed) integer from a kernel C function
> > > and converts it to an error if it's negative. Additionally, there are
> > > kernel C functions returning an unsigned integer where 0 is the error case.
> > > For example gen_pool_alloc() and friends. Provide a mechanism to convert
> > > this to Result too.
> > > 
> > > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> > > ---
> > >   rust/kernel/error.rs | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > I'm unsure if something like this is acceptable. Therefore the RFC.
> > > But I want at least ask ;) In the end this is a slightly modified
> > > copy of the existing to_result().
> > > 
> > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > index 376280b6a745a..3306b8f590866 100644
> > > --- a/rust/kernel/error.rs
> > > +++ b/rust/kernel/error.rs
> > > @@ -248,6 +248,16 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
> > >       }
> > >   }
> > > +/// Converts an unsigned integer as returned by a C kernel function to EINVAL if it's zero,
> > > +/// and `Ok(u64)` otherwise.
> > > +pub fn to_result_zero(val: core::ffi::c_ulong) -> Result<u64> {
> > > +    if val == 0 {
> > > +        Err(code::EINVAL)
> > > +    } else {
> > > +        Ok(val)
> > > +    }
> > > +}
> > 
> > How would this be used?
> 
> One (randomly selected) usage example:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/cpu/mce/genpool.c#n107
> 
> node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
> if (!node) {
> 	pr_warn_ratelimited("MCE records pool full!\n");
> 	return -ENOMEM;
> }
> 
> > 0 is normally not an error, why would you
> > need/want to turn that into an error value?

So you are treating NULL as 0 in rust bindings somehow?

I guess a usage of the new function would be good to see to clarify this
please.

thanks,

greg k-h

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Behme Dirk (CM/ESO2)]

On 11.01.2024 08:54, Greg KH wrote:
> On Thu, Jan 11, 2024 at 08:29:06AM +0100, Behme Dirk (CM/ESO2) wrote:
>> On 11.01.2024 07:59, Greg KH wrote:
>>> On Thu, Jan 11, 2024 at 07:44:15AM +0100, Dirk Behme wrote:
>>>> The existing to_result() takes a (signed) integer from a kernel C function
>>>> and converts it to an error if it's negative. Additionally, there are
>>>> kernel C functions returning an unsigned integer where 0 is the error case.
>>>> For example gen_pool_alloc() and friends. Provide a mechanism to convert
>>>> this to Result too.
>>>>
>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>> ---
>>>>    rust/kernel/error.rs | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> I'm unsure if something like this is acceptable. Therefore the RFC.
>>>> But I want at least ask ;) In the end this is a slightly modified
>>>> copy of the existing to_result().
>>>>
>>>> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
>>>> index 376280b6a745a..3306b8f590866 100644
>>>> --- a/rust/kernel/error.rs
>>>> +++ b/rust/kernel/error.rs
>>>> @@ -248,6 +248,16 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
>>>>        }
>>>>    }
>>>> +/// Converts an unsigned integer as returned by a C kernel function to EINVAL if it's zero,
>>>> +/// and `Ok(u64)` otherwise.
>>>> +pub fn to_result_zero(val: core::ffi::c_ulong) -> Result<u64> {
>>>> +    if val == 0 {
>>>> +        Err(code::EINVAL)
>>>> +    } else {
>>>> +        Ok(val)
>>>> +    }
>>>> +}
>>>
>>> How would this be used?
>>
>> One (randomly selected) usage example:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/cpu/mce/genpool.c#n107
>>
>> node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
>> if (!node) {
>> 	pr_warn_ratelimited("MCE records pool full!\n");
>> 	return -ENOMEM;
>> }
>>
>>> 0 is normally not an error, why would you
>>> need/want to turn that into an error value?
> 
> So you are treating NULL as 0 in rust bindings somehow?

gen_pool_alloc() returns an unsigned long. So we are talking about 0 
being the error case we need to catch via Result. Similar to the 
existing to_result():

/// 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_errno(err))
     } else {
         Ok(())
     }
}

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/error.rs#n241

> I guess a usage of the new function would be good to see to clarify this
> please.
Something like

let pool = to_result_zero(unsafe { bindings::gen_pool_alloc(pool_ptr, 
size) })?;

Best regards

Dirk

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Trevor Gross]

On Thu, Jan 11, 2024 at 3:14 AM Behme Dirk (CM/ESO2)
<dirk.behme@de.bosch.com> wrote:
>>>> How would this be used?
>>>
>>> One (randomly selected) usage example:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/cpu/mce/genpool.c#n107
>>>
>>> node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
>>> if (!node) {
>>>     pr_warn_ratelimited("MCE records pool full!\n");
>>>     return -ENOMEM;
>>> }
>>>
>>>> 0 is normally not an error, why would you
>>>> need/want to turn that into an error value?
>>
>> So you are treating NULL as 0 in rust bindings somehow?
>
> gen_pool_alloc() returns an unsigned long. So we are talking about 0
> being the error case we need to catch via Result. Similar to the
> existing to_result():
>
> /// 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_errno(err))
>     } else {
>         Ok(())
>     }
> }
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/error.rs#n241
>
> > I guess a usage of the new function would be good to see to clarify this
> > please.
> Something like
>
> let pool = to_result_zero(unsafe { bindings::gen_pool_alloc(pool_ptr,
> size) })?;

Are these cases mostly from where unsigned long is used as a pointer?
If so, maybe it would be convenient to return a `*mut u8` or
`NonNull<u8>`. It should probably take the error type as a parameter
so you can specify.

For what it's worth, you can also do this using NonZero* numbers or
NonNull pointer [1,2], with ok_or to provide the error type:

    let tmp = unsafe { bindings::gen_pool_alloc(pool_ptr, size) } as
*mut mce_evt_llist;
    let pool = NonNull::new(tmp).ok_or(ENOMEM)?;

- Trevor

> Best regards
>
> Dirk
>

[1]: https://doc.rust-lang.org/core/num/struct.NonZeroU64.html#method.new
[2]: https://doc.rust-lang.org/core/ptr/struct.NonNull.html#method.new

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Benno Lossin]

On 11.01.24 09:14, Behme Dirk (CM/ESO2) wrote:
> On 11.01.2024 08:54, Greg KH wrote:
>> On Thu, Jan 11, 2024 at 08:29:06AM +0100, Behme Dirk (CM/ESO2) wrote:
>>> On 11.01.2024 07:59, Greg KH wrote:
>>>> On Thu, Jan 11, 2024 at 07:44:15AM +0100, Dirk Behme wrote:
>>>>> The existing to_result() takes a (signed) integer from a kernel C function
>>>>> and converts it to an error if it's negative. Additionally, there are
>>>>> kernel C functions returning an unsigned integer where 0 is the error case.
>>>>> For example gen_pool_alloc() and friends. Provide a mechanism to convert
>>>>> this to Result too.
>>>>>
>>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>>> ---
>>>>>     rust/kernel/error.rs | 10 ++++++++++
>>>>>     1 file changed, 10 insertions(+)
>>>>>
>>>>> I'm unsure if something like this is acceptable. Therefore the RFC.
>>>>> But I want at least ask ;) In the end this is a slightly modified
>>>>> copy of the existing to_result().
>>>>>
>>>>> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
>>>>> index 376280b6a745a..3306b8f590866 100644
>>>>> --- a/rust/kernel/error.rs
>>>>> +++ b/rust/kernel/error.rs
>>>>> @@ -248,6 +248,16 @@ pub fn to_result(err: core::ffi::c_int) -> Result {
>>>>>         }
>>>>>     }
>>>>> +/// Converts an unsigned integer as returned by a C kernel function to EINVAL if it's zero,
>>>>> +/// and `Ok(u64)` otherwise.
>>>>> +pub fn to_result_zero(val: core::ffi::c_ulong) -> Result<u64> {
>>>>> +    if val == 0 {
>>>>> +        Err(code::EINVAL)
>>>>> +    } else {
>>>>> +        Ok(val)
>>>>> +    }
>>>>> +}
>>>>
>>>> How would this be used?
>>>
>>> One (randomly selected) usage example:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/cpu/mce/genpool.c#n107
>>>
>>> node = (void *)gen_pool_alloc(mce_evt_pool, sizeof(*node));
>>> if (!node) {
>>> 	pr_warn_ratelimited("MCE records pool full!\n");
>>> 	return -ENOMEM;
>>> }
>>>
>>>> 0 is normally not an error, why would you
>>>> need/want to turn that into an error value?
>>
>> So you are treating NULL as 0 in rust bindings somehow?
> 
> gen_pool_alloc() returns an unsigned long. 

Why does it return an unsigned long and not `void *`? I looked at some
other usages and I found several cases where the result is immediately
cast to a pointer. Maybe it might be a better idea to just change the
return type to a pointer type. If that is too much work though, I would
recommend to first turn it into a pointer on the Rust side and then
use `NonNull` or something similar like Trevor suggested.

--
Cheers,
Benno

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Alice Ryhl]

Dirk Behme <dirk.behme@de.bosch.com> writes:
> The existing to_result() takes a (signed) integer from a kernel C function
> and converts it to an error if it's negative. Additionally, there are
> kernel C functions returning an unsigned integer where 0 is the error case.
> For example gen_pool_alloc() and friends. Provide a mechanism to convert
> this to Result too.

Hmm. One difference between `to_result` and the proposed
`to_result_zero` is that `to_result` has logic to determine what kind of
error to return based on the value of the integer, whereas
`to_result_zero` is just hard-coded to always return a specific error
code. Furthermore, I find that usually for these functions, the error
code you want is not necessarily EINVAL. For example, when `kmalloc`
returns a null pointer, the error is ENOMEM, or when `fget` returns a
null pointer, the error is EBADF.

I think for this kind of use-case, we don't need a new function and can
`NonNull::new(ptr).ok_or(ENOMEM)?` to return an ENOMEM if the pointer is
null. Similarly, if it's an integer, we can use e.g. `NonZeroU32` in the
same way.

For an example of this pattern, see `File::fget` in [1].

Alice

[1]: https://lore.kernel.org/all/20231206-alice-file-v2-1-af617c0d9d94@google.com/

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Trevor Gross]

On Thu, Jan 11, 2024 at 7:18 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> Why does it return an unsigned long and not `void *`? I looked at some
> other usages and I found several cases where the result is immediately
> cast to a pointer. Maybe it might be a better idea to just change the
> return type to a pointer type. If that is too much work though, I would
> recommend to first turn it into a pointer on the Rust side and then
> use `NonNull` or something similar like Trevor suggested.
>
> --
> Cheers,
> Benno

I had to look this up too. Apparently that's an older use to prevent
accidental dereferencing [1] (?), and `unsigned long` of course
predates size_t/intptr_t.

[1]: https://static.lwn.net/images/pdf/LDD3/ch11.pdf

Re: [PATCH] [RFC] rust: error: Convert 0 being an error to Result

[Behme Dirk (CM/ESO2)]

On 11.01.2024 15:14, Alice Ryhl wrote:
> Dirk Behme <dirk.behme@de.bosch.com> writes:
>> The existing to_result() takes a (signed) integer from a kernel C function
>> and converts it to an error if it's negative. Additionally, there are
>> kernel C functions returning an unsigned integer where 0 is the error case.
>> For example gen_pool_alloc() and friends. Provide a mechanism to convert
>> this to Result too.
> 
> Hmm. One difference between `to_result` and the proposed
> `to_result_zero` is that `to_result` has logic to determine what kind of
> error to return based on the value of the integer, whereas
> `to_result_zero` is just hard-coded to always return a specific error
> code. Furthermore, I find that usually for these functions, the error
> code you want is not necessarily EINVAL. For example, when `kmalloc`
> returns a null pointer, the error is ENOMEM, or when `fget` returns a
> null pointer, the error is EBADF.
> 
> I think for this kind of use-case, we don't need a new function and can
> `NonNull::new(ptr).ok_or(ENOMEM)?` to return an ENOMEM if the pointer is
> null. Similarly, if it's an integer, we can use e.g. `NonZeroU32` in the
> same way.
> 
> For an example of this pattern, see `File::fget` in [1].
> 
> Alice
> 
> [1]: https://lore.kernel.org/all/20231206-alice-file-v2-1-af617c0d9d94@google.com/

So it seems min 3 people vote for NonNull usage :) Will do so. Let's 
drop this patch then.

Many thanks for all comments!

Best regards

Dirk