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

[Benno Lossin <benno.lossin@proton.me>]

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