On 05.08.24 12:55, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@xxxxxxxxx> 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? > For custom parsing functions implemented in the module, it makes sense > specifying this value in the trait. Couldn't I just emulate the behavior from above by creating a `struct MyI32(i32)` and having a default value? In that case, why make it more difficult instead of providing a simple way to do it? >> Since one might want an option by default be `true` and when one writes >> `my_module.param`, it should be `false`. > > I think this would be confusing, since the default C parameter types do > not have this semantic. Specifying a default true boolean as an argument > does not make it false in C land. TBH this also feels like the most sane > thing to do. Can't you also do the custom parameter parsing from above? ie have an integer parameter with a default value? Or is that frowned upon (ie not allowed through review)? >> Also as the C side shows, having default values for integer types is not >> really a good idea, since you might want a non-zero default value. >> If one does not specify the `default_active` value, then the >> `KERNEL_PARAM_OPS_FL_NOARG` is not set. > > I would rather predefine `KERNEL_PARAM_OPS_FL_NOARG` in the trait > implementation like C does. We would set the flag for `bool` but not for > integer types. For custom implementations of the trait, the developer > can do whatever they want. So we are only going to use it for `bool`? >> If you don't want to implement this (which I can fully understand, since >> we might get `syn` before anyone needs params with default values), then >> we should write this idea down (maybe in an issue?). But regardless, I >> would like to know your opinion on this topic. > > We can create he issue if this series is accepted without the feature. > >> >>>>> I think we need to drop the default value if we adopt the arg without >>>>> value scheme. >>>> >>>> Yes definitely. I don't see anything in the code doing this currently, >>>> right? >>> >>> The current patch uses the default value given in the `module!` macro to >>> initialize the parameter value. >> >> But what drops the default value, when an actual value is specified via >> `my_module.param=value`? (or is the default value only assigned when >> nothing is specified?) > > Some more confusion maybe. When I wrote "drop the default value", I > meant remove it from the code, not specify it. I take it back though. We > would use the value given in the `module!` macro `default` field as > `default_inactive`. `default_active` is not required for integer types, > since they always require a value for the argument. > > But the drop would happen in `set_param` when the return value of > `core::ptr::replace` is dropped. Ah I see, I missed that. >>>>> We could also only allow `Copy` values to be used as Parameters (but >>>> then `str` cannot be used as a parameter...), since they can't implement >>>> `Drop`. >>> >>> We should plan on eventually supporting `String`. >> >> Yes. >> >>>>>>> Or we could just not adopt this feature in the Rust abstractions. >>>>>> >>>>>> Depends on how common this is and if people need to use it. I think that >>>>>> what I proposed above isn't that complex, so it should be easy to >>>>>> implement. >>>>> >>>>> Rust modules would just force people to add "my_module.param=1" instead >>>>> of just "my_module.param". I think that is reasonable. >>>> >>>> Eh, why do we want to give that up? I don't think it's difficult to do. >>> >>> I just don't see the point. Just have the user pass `my_module.param=1` >>> instead of omitting the value. Having multiple ways of specifying for >>> instance a true boolean just leads to confusion. Some boolean parameters >>> have a default value of `true`, for instance `nvme.use_cmb_sqes`. In >>> this case specifying `nvme.use_cmb_sqes` has no effect, even though one >>> might think it has. >> >> This just shows to me that a "global" default in `ModuleParam` is wrong, >> since for `use_cmb_sqes` one could either have a negated flag, or no >> default value, forcing the user to write `nvme.use_cmb_sqes=false`. > > I do not think it is a good idea to be able to override the value > assigned to a parameter when it is given without a value. The more > predictable a user interface is, the better. By that argument, we should also not permit custom implementations to specify a default value. --- Cheers, Benno
