"Miguel Ojeda" <miguel.ojeda.sandonis@xxxxxxxxx> writes:
> On Tue, Jul 1, 2025 at 5:43 PM Benno Lossin <lossin@xxxxxxxxxx> wrote:
>>
>> Ultimately this is something for Miguel to decide.
>
> Only if you all cannot get to an agreement ;)
>
> If Andreas wants to have it already added, then I would say just mark
> it `unsafe` as Benno recommends (possibly with an overbearing
> precondition), given it has proven subtle/forgettable enough and that,
> if I understand correctly, it would actually become unsafe if someone
> "just" added "reasonably-looking code" elsewhere.
You are right that if someone added code to the API, the API could
become unsound. But that is the deal with all our APIs and I don't agree
that the details are very subtle here. Someone would need to add sysfs
support or user provided parameter parsing to cause the unsoundness we
are talking about.
Anyone attempting such a task should have proper understanding of the
code first, and given the ample amount of `NOTE` comments I have added,
it should be clear that the concurrent accesses that this addition would
introduce, needs to be accounted for, to avoid data races.
I will add myself as a reviewer for the rust module parameter parsing
code if that is OK with module maintainers.
> That way we have an incentive to make it safe later on and, more
> importantly, to think again about it when such a patch lands,
> justifying it properly. And it could plausibly protect out-of-tree
> users, too.
Again, I do not think it is reasonable to mark this function unsafe.
> This is all assuming that we will not have many users of this added
> right away (in a cycle or two), i.e. assuming it will be easy to
> change callers later on (if only to remove the `unsafe {}`).
rnull will use this the cycle after it is merged.
Best regards,
Andreas Hindborg
