On 01.08.24 15:40, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>> On 01.08.24 13:29, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>>>> On 05.07.24 13:15, Andreas Hindborg wrote:
>>>>> +
>>>>> +/// Types that can be used for module parameters.
>>>>> +///
>>>>> +/// Note that displaying the type in `sysfs` will fail if
>>>>> +/// [`core::str::from_utf8`] (as implemented through the [`core::fmt::Display`]
>>>>> +/// trait) writes more than [`PAGE_SIZE`] bytes (including an additional null
>>>>> +/// terminator).
>>>>> +///
>>>>> +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE`
>>>>> +pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
>>>>> + /// The `ModuleParam` will be used by the kernel module through this type.
>>>>> + ///
>>>>> + /// This may differ from `Self` if, for example, `Self` needs to track
>>>>> + /// ownership without exposing it or allocate extra space for other possible
>>>>> + /// parameter values. This is required to support string parameters in the
>>>>> + /// future.
>>>>> + type Value: ?Sized;
>>>>> +
>>>>> + /// Whether the parameter is allowed to be set without an argument.
>>>>> + ///
>>>>> + /// Setting this to `true` allows the parameter to be passed without an
>>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`).
>>>>> + const NOARG_ALLOWED: bool;
>>>>
>>>> I think, there is a better way of doing this. Instead of this bool, we
>>>> do the following:
>>>> 1. have a `const DEFAULT: Option<Self>`
>>>> 2. change the type of the argument of `try_from_param_arg` to
>>>> `&'static [u8]`
>>>>
>>>> That way we don't have the weird behavior of `try_from_param_arg` that
>>>> for params that don't have a default value.
>>>
>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in
>>> this patch set, it is effectively dead code. I will remove it.
>>
>> Hmm what parameters actually are optional? I looked at the old rust
>> branch and only `bool` is marked as optional. Are there others?
>>
>> If it is used commonly for custom parameters (I could imagine that Rust
>> modules have enums as parameters and specifying nothing could mean the
>> default value), then it might be a good idea to just include it now.
>> (otherwise we might forget the design later)
>
> As far as I can tell from the C code, all parameters are able to have
> the `NOARG` flag set. We get a null pointer in the callback in that
> case.
>
> If we want to handle this now, we could drop the `default` field
> in the Rust module macro. There is no equivalent in the C macros.
> And then use an `Option<Option<_>>` to represent the value. `None` would
> be an unset parameter. `Some(None)` would be a parameter without a
> value. `Some(Some(_))` would be a set parameter with a value. We could
> probably fix the types so that only parameters with the `NOARG` flag use
> the double option, others use a single option.
What did you think of my approach that I detailed above? I would like to
avoid `Option<Option<_>>` if we can.
> 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.
>>>>> + param_type.to_string(),
>>>>> + param_ops_path(¶m_type).to_string(),
>>>>> + );
>>>>> +
>>>>> + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type);
>>>>
>>>> Is the spelling intentional? "parmtype"?
>>>
>>> This is intentional. I don't think the kernel is ever parsing this, but
>>> it is parsed by the `modinfo` tool.
>>
>> Hmm, why is it not `paramtype`? Does that conflict with something?
>
> You would have to take that up with the maintainer(s) of the `modinfo`
> tool. The name is externally dictated [1].
Thanks for the pointer that's what I wanted to know (is it given from
somewhere else? or is it a name that we came up with), then it's fine :)
>>>>> + // Note: when we enable r/w parameters, we need to lock here.
>>>>> +
>>>>> + // SAFETY: Parameters do not need to be locked because they are
>>>>> + // read only or sysfs is not enabled.
>>>>> + unsafe {{
>>>>> + <{param_type_internal} as kernel::module_param::ModuleParam>::value(
>>>>> + &__{name}_{param_name}_value
>>>>> + )
>>>>> + }}
>>>>> + }}
>>>>> + ",
>>>>> + name = info.name,
>>>>> + param_name = param_name,
>>>>> + param_type_internal = param_type_internal,
>>>>> + );
>>>>> +
>>>>> + let kparam = format!(
>>>>> + "
>>>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{
>>>>> + // SAFETY: Access through the resulting pointer is
>>>>> + // serialized by C side and only happens before module
>>>>> + // `init` or after module `drop` is called.
>>>>> + arg: unsafe {{ &__{name}_{param_name}_value }}
>>>>> + as *const _ as *mut core::ffi::c_void,
>>>>
>>>> Here you should use `addr_of[_mut]!` instead of taking a reference.
>>>
>>> This is a static initializer, so it would be evaluated in const context.
>>> At that time, this is going to be the only reference to
>>> `&__{name}_{param_name}_value` which would be const. So it should be
>>> fine?
>>
>> When compiling this [1] with a sufficiently new Rust version, you will
>> get an error:
>>
>> warning: creating a shared reference to mutable static is discouraged
>> --> src/main.rs:4:22
>> |
>> 4 | let x = unsafe { &foo };
>> | ^^^^ shared reference to mutable static
>> |
>> = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
>> = note: this will be a hard error in the 2024 edition
>> = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
>> = note: `#[warn(static_mut_refs)]` on by default
>> help: use `addr_of!` instead to create a raw pointer
>> |
>> 4 | let x = unsafe { addr_of!(foo) };
>> | ~~~~~~~~~~~~~
>>
>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1
>>
>> So I think we should start using `addr_of!` for mutable static now.
>
> Oh. Thanks for the pointer.
>
> Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes
> away as well with the feature you linked as well.
I think that will take some time until it is gone.
> This also requires `const_mut_refs`, but as I recall that is going to be
> stabilized soon.
That should only be needed if you need `addr_of_mut!`, but IIUC, you
only need `addr_of!`, right?
---
Cheers,
Benno
