Re: [PATCH] rust: add `module_params` macro

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

On 15.08.24 15:11, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>> On 05.08.24 12:55, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>> On 02.08.24 12:27, Andreas Hindborg wrote:
>>>>> At a higher level where the bindings supply the parsing functions, we
>>>>> can decide that passing an argument without a value yields a default
>>>>> parameter value. C does this for the predefined `bool` type. The
>>>>> predefined integer types does not support omitting the value.
>>>>>
>>>>> This patch only supports the higher level predefined parameter types,
>>>>> and does not allow modules to supply their own parameter parsing
>>>>> functions. None of the types we implement in this patch support passing
>>>>> the argument without a value. This is intentional to mirror the C
>>>>> implementation.
>>>>>
>>>>> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function
>>>>> trait to:
>>>>>
>>>>>     fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>;
>>>>>
>>>>> If/when we start supporting types like `bool` or custom parsing
>>>>> functions provided by the module, we will have to update the signature
>>>>> to take an `Option` to represent the case where the user passed an
>>>>> argument without a value. However, to mimic C, the function must always
>>>>> return a value if successful, even if the user did not supply a value to
>>>>> the argument.
>>>>>
>>>>> Two different default values are in flight here. 1) the value that the
>>>>> parameter will have before the kernel calls `try_from_param_arg` via
>>>>> `set_param` and 2) the value to return from `try_from_param_arg` if the
>>>>> user did not pass a value with the argument.
>>>>>
>>>>> For a `bool` 1) would usually be `false` and 2) would always be `true`.
>>>>>
>>>>> For predefined types the module would not customize 2), but 1) is useful
>>>>> to customize. For custom types where the module supplies the parsing
>>>>> function, 2) would be implicitly given by the module in the parsing
>>>>> function.
>>>>>
>>>>> In this patch set, we only have 1 default value, namely 1). We do not
>>>>> need 2) because we do not support parameters without values.
>>>>
>>>> I am not sure that putting the default value of `my_module.param` into
>>>> the `ModuleParam` trait is a good idea. It feels more correct to me to
>>>> add an optional field to the part in `module!` that can be set to denote
>>>> this default value -- we might also want to change the name of
>>>> `default`, what do you think of `default_inactive` and `default_active`?
>>>
>>> For all the predefined parameter types, the module code would never set
>>> the `default_active` value. It should be part of the data parsing
>>> specification for the predefined argument types.
>>
>> So if your module has an i32 parameter, you can't set a default value to
>> eg 1000?
> 
> You _would_ be able to set the `default_inactive` value, which is the
> value assigned to the static at initialization time. It would make sense
> to default this to 0 for integer types and make it overridable in the
> `module!` macro.

Hmm, I would say it makes more sense to have the user always specify a
default in `module!`, since integers can mean a lot of different things.

> You would not be to set the `default_active` value, which is the value
> assigned to the parameter static variable, when the parameter is passed
> without value. The reason being that we want to mirror C, so we prohibit
> this for predefined integer parameter types.

Gotcha, I wasn't 100% sure in our previous emails if we wanted to
exactly mirror C or not. Should've asked that.
Thanks for taking the time to write this clarification (and the other
ones as well!), I think I now finally understand why you do it this way.
I agree with your approach.

---
Cheers,
Benno