"Benno Lossin" <lossin@xxxxxxxxxx> writes:
> On Tue Jul 1, 2025 at 4:14 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@xxxxxxxxxx> writes:
>>> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>>>> No, I am OK for now with configfs.
>>>>
>>>> But, progress is still great. How about if we add a copy accessor
>>>> instead for now, I think you proposed that a few million emails ago:
>>>>
>>>> pub fn get(&self) -> T;
>>>>
>>>> or maybe rename:
>>>>
>>>> pub fn copy(&self) -> T;
>>>>
>>>> Then we are fine safety wise for now, right? It is even sensible for
>>>> these `T: Copy` types.
>>>
>>> That is better than getting a reference, but still someone could read at
>>> the same time that a write is happening (though we need some new
>>> abstractions AFAIK?). But I fear that we forget about this issue,
>>> because it'll be some time until we land parameters that are `!Copy` (if
>>> at all...)
>>
>> No, that could not happen when we are not allowing custom parsing or
>> sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
>> and I would add one on this issue as well.
>
> Ultimately this is something for Miguel to decide. I would support an
> unsafe accessor (we should also make it `-> T`), since there it "can't
> go wrong", any UB is the fault of the user of the API. It also serves as
> a good reminder, since a `NOTE` comment shouldn't be something
> guaranteeing safety (we do have some of these global invariants, but I
> feel like this one is too tribal and doesn't usually come up, so I feel
> like it's more dangerous).
I see no reason for making it unsafe when it is safe?
/// Get a copy of the parameter value.
///
/// # Note
///
/// When this method is called in `const` context, the default
/// parameter value will be returned. During module initialization, the
/// kernel will populate the parameter with the value supplied at module
/// load time or kernel boot time.
// NOTE: When sysfs access to parameters are enabled, we have to pass in a
// held lock guard here.
//
// NOTE: When we begin supporting custom parameter parsing with user
// supplied code, this method must be synchronized.
pub const fn copy(&self) -> T {
// SAFETY: As we only support read only parameters with no sysfs
// exposure, the kernel will not touch the parameter data after module
// initialization. The kernel will update the parameters serially, so we
// will not race with other accesses.
unsafe { *self.data.get() }
}
>
> I think we should also move this patchset along, we could also opt for
> no accessor at all :) Then it isn't really useful without the downstream
> accessor function, but that can come after.
I would rather not merge it without an access method. Then it is just
dead code, and we will not notice if we break it.
Best regards,
Andreas Hindborg
