Re: [RFC PATCH] rust: types: Add explanation for ARef pattern

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

On 29.07.24 13:31, Alice Ryhl wrote:
> On Fri, Jul 26, 2024 at 6:20 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Fri, Jul 26, 2024 at 03:54:37PM +0000, Benno Lossin wrote:
>>> On 26.07.24 17:15, Boqun Feng wrote:
>>>> On Fri, Jul 26, 2024 at 02:42:36PM +0000, Benno Lossin wrote:
>>>>> On 26.07.24 16:26, Boqun Feng wrote:
>>>>>> On Fri, Jul 26, 2024 at 01:43:38PM +0000, Benno Lossin wrote:
>>>>>> [...]
>>>>>>>>>
>>>>>>>>> You can always get a `&T` from `ARef<T>`, since it implements `Deref`.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yeah, but this is unrelated. I was talking about that API providers can
>>>>>>>> decide whether they want to only provide a `raw_ptr` -> `ARef<Self>` if
>>>>>>>> they don't need to provide a `raw_ptr` -> `&Self`.
>>>>>>>>
>>>>>>>>>> Overall, I feel like we don't necessarily make a preference between
>>>>>>>>>> `->&Self` and `->ARef<Self>` functions here, since it's up to the users'
>>>>>>>>>> design?
>>>>>>>>>
>>>>>>>>> I would argue that there should be a clear preference for functions
>>>>>>>>> returning `&Self` when possible (ie there is a parameter that the
>>>>>>>>
>>>>>>>> If "possible" also means there's going to be `raw_ptr` -> `&Self`
>>>>>>>> function (as the same publicity level) anyway, then agreed. In other
>>>>>>>> words, if the users only need the `raw_ptr` -> `ARef<Self>`
>>>>>>>> functionality, we don't want to force people to provide a `raw_ptr` ->
>>>>>>>> `&Self` just because, right?
>>>>>>>
>>>>>>> I see... I am having a hard time coming up with an example where users
>>>>>>> would exclusively want `ARef<Self>` though... What do you have in mind?
>>>>>>> Normally types wrapped by `ARef` have `&self` methods.
>>>>>>>
>>>>>>
>>>>>> Having `&self` methods doesn't mean the necessarity of a `raw_ptr` ->
>>>>>> `&Self` function, for example, a `Foo` is wrapped as follow:
>>>>>>
>>>>>>   struct Foo(Opaque<foo>);
>>>>>>   impl Foo {
>>>>>>       pub fn bar(&self) -> Bar { ... }
>>>>>>       pub unsafe fn get_foo(ptr: *mut foo) -> ARef<Foo> { ... }
>>>>>>   }
>>>>>>
>>>>>> in this case, the abstration provider may not want user to get a
>>>>>> `raw_ptr` -> `&Self` function, so no need to have it.
>>>>>
>>>>> I don't understand this, why would the abstraction provider do that? The
>>>>
>>>> Because no user really needs to convert a `raw_ptr` to a `&Self` whose
>>>> lifetime is limited to a scope?
>>>
>>> What if you have this:
>>>
>>>     unsafe extern "C" fn called_from_c_via_vtable(foo: *mut bindings::foo) {
>>>         // SAFETY: ...
>>>         let foo = unsafe { Foo::from_raw(foo) };
>>>         foo.bar();
>>>     }
>>>
>>> In this case, there is no need to take a refcount on `foo`.
>>>
>>>> Why do we provide a function if no one needs and the solely purpose is
>>>> to just avoid providing another function?
>>>
>>> I don't think that there should be a lot of calls to that function
>>> anyways and thus I don't think there is value in providing two functions
>>> for almost the same behavior. Since one can be derived by the other, I
>>> would go for only implementing the first one.
>>
>> I don't think there should be a rule saying that we can't provide a wrapper
>> function for deriving an `ARef<T>`. `Device` is a good example:
>>
>> `let dev: ARef<Device> = unsafe { Device::from_raw(raw_dev) }.into();`
>>
>> vs.
>>
>> `let dev = unsafe { Device::get(raw_dev) };`
>>
>> To me personally, the latter looks quite a bit cleaner.
>>
>> Besides that, I think every kernel engineer (even without Rust background) will
>> be able to decode the meaning of this call. And if we get the chance to make
>> things obvious to everyone *without* the need to make a compromise, we should
>> clearly take it.
> 
> I think I've come around on this question. I think it's fine to have
> raw_ptr->ARef methods that increment the refcount, but we should make
> a naming convention clear. I propose:
> 
> * Functions named things like from_raw_file or from_raw_mm do not
> increment the refcount.
> * Functions named things like get_file or or mmget do increment the
> refcount, just like the C function of the same name.

I have thought about this a bit and I think that we can try to do it. I
like the name `Device::get` and `Device::from_raw`. I would not
duplicate the name ie `Device::get_device` (nor would I do that with
`from_raw`).

One of my bigger problems was the naming, so it's good to see this.

---
Cheers,
Benno