Date:   Mon, 23 Oct 2023 17:19:12 +0000
From:   Benno Lossin <benno.lossin@...ton.me>
To:     "Andreas Hindborg (Samsung)" <nmi@...aspace.dk>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Alice Ryhl <aliceryhl@...gle.com>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

On 23.10.23 10:30, Andreas Hindborg (Samsung) wrote:
> 
> Benno Lossin <benno.lossin@...ton.me> writes:
> 
>> On 20.10.23 11:06, Andreas Hindborg (Samsung) wrote:
>>> Benno Lossin <benno.lossin@...ton.me> writes:
>>>> +/// Error message for calling a default function of a [`#[vtable]`](macros::vtable) trait.
>>>> +pub const VTABLE_DEFAULT_ERROR: &str =
>>>> +    "This function must not be called, see the #[vtable] documentation.";
>>>> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
>>>> index c42105c2ff96..daf1ef8baa62 100644
>>>> --- a/rust/macros/lib.rs
>>>> +++ b/rust/macros/lib.rs
>>>> @@ -87,27 +87,41 @@ pub fn module(ts: TokenStream) -> TokenStream {
>>>>    /// implementation could just return `Error::EINVAL`); Linux typically use C
>>>>    /// `NULL` pointers to represent these functions.
>>>>    ///
>>>> -/// This attribute is intended to close the gap. Traits can be declared and
>>>> -/// implemented with the `#[vtable]` attribute, and a `HAS_*` associated constant
>>>> -/// will be generated for each method in the trait, indicating if the implementor
>>>> -/// has overridden a method.
>>>> +/// This attribute closes that gap. A trait can be annotated with the `#[vtable]` attribute.
>>>> +/// Implementers of the trait will then also have to annotate the trait with `#[vtable]`. This
>>>> +/// attribute generates a `HAS_*` associated constant bool for each method in the trait that is set
>>>> +/// to true if the implementer has overridden the associated method.
>>>> +///
>>>> +/// For a function to be optional, it must have a default implementation. But this default
>>>> +/// implementation will never be executed, since these functions are exclusively called from
>>>> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
>>>> +/// will execute the default behavior. Since it is not maintainable to replicate the default
>>>> +/// behavior in Rust, the default implementation should be:
>>>
>>> How about this?:
>>>
>>> For a Rust trait method to be optional, it must have a default
>>> implementation. For a trait marked with `#[vtable]`, the default
>>> implementation will not be executed, as the only way the trait methods
>>> should be called is through function pointers installed in C side
>>> vtables. When a trait implementation marked with `#[vtable]` is missing
>>> a method, a `NULL` pointer will be installed in the corresponding C side
>>> vtable, and thus the Rust default implementation can not be called. The
>>> default implementation should be:
>>>
>>> Not sure if it is more clear 🤷
>>
>> I think it misses the following important point: why is it not
>> possible to just replicate the default behavior?
>>
>> What do you think of this?:
>>
>> For a trait method to be optional, it must have a default implementation.
>> This is also the case for traits annotated with `#[vtable]`, but in this
>> case the default implementation will never be executed. The reason for this
>> is that the functions will be called through function pointers installed in
>> C side vtables. When an optional method is not implemented on a `#[vtable]`
>> trait, a NULL entry is installed in the vtable. Thus the default
>> implementation is never called. Since these traits are not designed to be
>> used on the Rust side, it should not be possible to call the default
>> implementation.
> 
>> It is not possible to replicate the default behavior from C
>> in Rust, since that is not maintainable.
> 
> I don't feel that this bit should be included. It's not a matter of
> maintainability. Why would we reimplement something that is already
> present in a subsystem? The functionality is already present, so we use
> it.

But we don't use it on the Rust side. You cannot write this:

     fn foo<T: Operations>(t: &T) -> Result<usize> {
         t.seek(0)?
     }

if the `seek` function is optional.

-- 
Cheers,
Benno

Powered by blists - more mailing lists