Message-ID: <au3adook2dsnveqlz3i7inmbzx7b5sel7qg2cn7efcacicthiy@fm5io4kam3s2>
Date:   Thu, 26 Oct 2023 21:12:31 +0000
From:   "Ariel Miculas (amiculas)" <amiculas@...co.com>
To:     Benno Lossin <benno.lossin@...ton.me>
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>,
        Andreas Hindborg <a.hindborg@...sung.com>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>,
        Asahi Lina <lina@...hilina.net>,
        Sven Van Asbroeck <thesven73@...il.com>,
        Viktor Garske <viktor@...ar.de>, Finn Behrens <me@...enk.dev>,
        "rust-for-linux@...r.kernel.org" <rust-for-linux@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] rust: macros: improve `#[vtable]` documentation

On 23/10/26 08:19PM, Benno Lossin wrote:
> Traits marked with `#[vtable]` need to provide default implementations
> for optional functions. The C side represents these with `NULL` in the
> vtable, so the default functions are never actually called. We do not
> want to replicate the default behavior from C in Rust, because that is
> not maintainable. Therefore we should use `build_error` in those default
> implementations. The error message for that is provided at
> `kernel::error::VTABLE_DEFAULT_ERROR`.
> 
> Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
> ---
> v2 -> v3:
> - don't hide the import of the constant in the example
> - fixed "function" typo
> - improve paragraph about optional functions
> - do not remove the `Send + Sync + Sized` bounds on the example
> 
> v1 -> v2:
> - removed imperative mode in the paragraph describing optional
>   functions.
> 
>  rust/kernel/error.rs |  4 ++++
>  rust/macros/lib.rs   | 37 ++++++++++++++++++++++++++++++-------
>  2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 05fcab6abfe6..1373cde025ef 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -335,3 +335,7 @@ pub(crate) fn from_result<T, F>(f: F) -> T
>          Err(e) => T::from(e.to_errno() as i16),
>      }
>  }
> +
> +/// 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..917a51183c23 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -87,27 +87,48 @@ 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.

The above paragraph seems to be wrapped at 100 characters while the
paragraph below seems to be wrapped at 80 characters.

Cheers,
Ariel
> +///
> +/// 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. This is done to ensure that we call the vtable methods
> +/// through the C vtable, and not through the Rust vtable. Therefore, the
> +/// default implementation should call `kernel::build_error`, which prevents
> +/// calls to this function at compile time:
> +///
> +/// ```compile_fail
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
> +/// kernel::build_error(VTABLE_DEFAULT_ERROR)
> +/// ```
> +///
> +/// note that you might need to import [`kernel::error::VTABLE_DEFAULT_ERROR`].
>  ///
> -/// This attribute is not needed if all methods are required.
> +/// This macro should not be used when all functions are required.
>  ///
>  /// # Examples
>  ///
>  /// ```ignore
> +/// use kernel::error::VTABLE_DEFAULT_ERROR;
>  /// use kernel::prelude::*;
>  ///
>  /// // Declares a `#[vtable]` trait
>  /// #[vtable]
>  /// pub trait Operations: Send + Sync + Sized {
>  ///     fn foo(&self) -> Result<()> {
> -///         Err(EINVAL)
> +///         kernel::build_error(VTABLE_DEFAULT_ERROR)
>  ///     }
>  ///
>  ///     fn bar(&self) -> Result<()> {
> -///         Err(EINVAL)
> +///         kernel::build_error(VTABLE_DEFAULT_ERROR)
>  ///     }
>  /// }
>  ///
> @@ -125,6 +146,8 @@ pub fn module(ts: TokenStream) -> TokenStream {
>  /// assert_eq!(<Foo as Operations>::HAS_FOO, true);
>  /// assert_eq!(<Foo as Operations>::HAS_BAR, false);
>  /// ```
> +///
> +/// [`kernel::error::VTABLE_DEFAULT_ERROR`]: ../kernel/error/constant.VTABLE_DEFAULT_ERROR.html
>  #[proc_macro_attribute]
>  pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
>      vtable::vtable(attr, ts)
> 
> base-commit: 3857af38e57a80b15b994e19d1f4301cac796481
> -- 
> 2.41.0
> 
> 

Powered by blists - more mailing lists