Message ID | 20231019171540.259173-1-benno.lossin@proton.me |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2010:b0:403:3b70:6f57 with SMTP id fe16csp537681vqb; Thu, 19 Oct 2023 10:16:40 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEl9FSfhxqSV8hIcMaXCbM3F029PqcyfYEw56fBoWLp3z5PUkOlt/YQYbpNh0BaXl6qJUdx X-Received: by 2002:a17:902:f685:b0:1c8:791c:d797 with SMTP id l5-20020a170902f68500b001c8791cd797mr3228692plg.56.1697735799771; Thu, 19 Oct 2023 10:16:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697735799; cv=none; d=google.com; s=arc-20160816; b=gnkgoOmQ/sjRsUrQZj7kUHuflarbofVrgDPqkqGM2VWGr7XfknDAMFN4EcEKpnLyM9 fo51LC/8Udb7tHpGsDaB/lslVMngXggp2wt8Acwb2OK1bEZBSxmsdLLGEG1lUlcF+Crg L0ewpM3PqorLS6BEsq0ypPYwhzcAHnofGat6p7mDxBeu45xoyzR+e+ZQiHiWrZ7XBqh5 zzcxX1tXWmJGLp1hVI97xBwKKg2OWxTu5EOGanPnOVWm6ZErPN2zdYl/599MHgXU17mB zaZ89EPXm8pIb+ec+6wRrdoMgXqdt3UR2d3q6WU+06ydBafWulNeUipRxa8LJsxrL8h1 7JcA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :feedback-id:message-id:subject:cc:from:to:date:dkim-signature; bh=w1TaW2ie2JiPlJ9irZEgwnd/XHXqHoBuv2KG6JL9RIk=; fh=V11RwCzO6FhrwwR400IeaCztZOiMPvinK8BR27/cP4s=; b=dkA03PQ6wiZ2KHpwcmeBXrc7sFCLoa0IxLNsnaRWTkkzwz0+8OIFEwAPEhEoT/XG44 1n8AzRus99ZBQAvR4tWJSncE4hLwajtue3RQ2PpC3fIAlrzE1wNhRqQ7VNCfTmKAS8G4 LfVCEPeL9wVociT+LbX6kC1Fp4Q4cAa+s352N8s8nyGDdMznhjT4HX0dQoIzcZWYHM/i bGgzXRN/gRN1znEz5tUleU76pqFaWwwzFIyfEBXhdglPkR2Fa7MBDdasnSH9/5FKAcBC bBeOYitHzed23R8IN1YwtIWKmZMXUkyDlxSb6ASqhm4B3wSPj0zbE4hxCEQpgvlnaiBU Lo2g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@proton.me header.s=protonmail header.b=DxYTL6RN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=proton.me Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id p8-20020a170902a40800b001c878b52cf4si816400plq.365.2023.10.19.10.16.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Oct 2023 10:16:39 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@proton.me header.s=protonmail header.b=DxYTL6RN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=proton.me Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 7EAA881B454C; Thu, 19 Oct 2023 10:16:37 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233245AbjJSRQU (ORCPT <rfc822;a1648639935@gmail.com> + 26 others); Thu, 19 Oct 2023 13:16:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41908 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233017AbjJSRQS (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 19 Oct 2023 13:16:18 -0400 Received: from mail-40131.protonmail.ch (mail-40131.protonmail.ch [185.70.40.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F030CCF; Thu, 19 Oct 2023 10:16:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1697735770; x=1697994970; bh=w1TaW2ie2JiPlJ9irZEgwnd/XHXqHoBuv2KG6JL9RIk=; h=Date:To:From:Cc:Subject:Message-ID:Feedback-ID:From:To:Cc:Date: Subject:Reply-To:Feedback-ID:Message-ID:BIMI-Selector; b=DxYTL6RNUPIK+Pkqg7PZhn5B1I/ZpvkB4MkGcHjo25tMOxKN+Qs9lWrTAaCriWePB Kpl6+tLqx6CHEUoUBAfLi/wPDhHdAYLRIPRY5hz0kDtUbryQlTbro3BTazNY7xKg1h 2etDSAcayiLP95yAyIUJxaZuFM0iGkPwQuF+teaJ1Q32iL1tL3ev+otHyLudpYkvu2 t84xY+bZidkLwGS3MZ4HFzwe0fRtNRU0x4RYO1SBLG1PZbw5IEx+PQcACBDTiEb8zi 0ZXXWc2ICPgYQONrZ9mbJvfunyZvWgl0/KWTR4XCaAgRFkdyycNC57sWL4/dDsyE/k u3IkKr8MkSOXw== Date: Thu, 19 Oct 2023 17:15:53 +0000 To: Miguel Ojeda <ojeda@kernel.org>, Wedson Almeida Filho <wedsonaf@gmail.com>, Alex Gaynor <alex.gaynor@gmail.com> From: Benno Lossin <benno.lossin@proton.me> Cc: Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Benno Lossin <benno.lossin@proton.me>, Alice Ryhl <aliceryhl@google.com>, Andreas Hindborg <nmi@metaspace.dk>, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2] rust: macros: improve `#[vtable]` documentation Message-ID: <20231019171540.259173-1-benno.lossin@proton.me> Feedback-ID: 71780778:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 19 Oct 2023 10:16:37 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780205013568358850 X-GMAIL-MSGID: 1780205013568358850 |
Series |
[v2] rust: macros: improve `#[vtable]` documentation
|
|
Commit Message
Benno Lossin
Oct. 19, 2023, 5:15 p.m. UTC
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@proton.me>
---
v1 -> v2:
- removed imperative mode in the paragraph describing optional
functions.
rust/kernel/error.rs | 4 ++++
rust/macros/lib.rs | 32 ++++++++++++++++++++++++--------
2 files changed, 28 insertions(+), 8 deletions(-)
base-commit: a7135d10754760f0c038497b44c2c2f2b0fb5651
Comments
On 23/10/19 05:15PM, 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@proton.me> > --- > v1 -> v2: > - removed imperative mode in the paragraph describing optional > functions. > > rust/kernel/error.rs | 4 ++++ > rust/macros/lib.rs | 32 ++++++++++++++++++++++++-------- > 2 files changed, 28 insertions(+), 8 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..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: > +/// > +/// ```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 function are required. Typo: should be `all functions` > /// > /// # Examples > /// > /// ```ignore > +/// # use kernel::error::VTABLE_DEFAULT_ERROR; > /// use kernel::prelude::*; > /// > /// // Declares a `#[vtable]` trait > /// #[vtable] > -/// pub trait Operations: Send + Sync + Sized { > +/// pub trait Operations { > /// 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 +139,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: a7135d10754760f0c038497b44c2c2f2b0fb5651 > -- > 2.41.0 > > With the above typo fixed, Reviewed-by: Ariel Miculas <amiculas@cisco.com>
Benno Lossin <benno.lossin@proton.me> writes: > 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@proton.me> > --- > v1 -> v2: > - removed imperative mode in the paragraph describing optional > functions. > > rust/kernel/error.rs | 4 ++++ > rust/macros/lib.rs | 32 ++++++++++++++++++++++++-------- > 2 files changed, 28 insertions(+), 8 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..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 🤷 > +/// > +/// ```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 function are required. > /// > /// # Examples > /// > /// ```ignore > +/// # use kernel::error::VTABLE_DEFAULT_ERROR; > /// use kernel::prelude::*; > /// > /// // Declares a `#[vtable]` trait > /// #[vtable] > -/// pub trait Operations: Send + Sync + Sized { > +/// pub trait Operations { > /// 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 +139,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: a7135d10754760f0c038497b44c2c2f2b0fb5651
On 10/19/23 19:15, 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@proton.me> Reviewed-by: Alice Ryhl <aliceryhl@google.com> There is a minor nit below, and in reviews sent by others, but overall this looks fine to me. > /// # Examples > /// > /// ```ignore > +/// # use kernel::error::VTABLE_DEFAULT_ERROR; > /// use kernel::prelude::*; I probably wouldn't hide the import of VTABLE_DEFAULT_ERROR from this example.
On 20.10.23 11:06, Andreas Hindborg (Samsung) wrote: > Benno Lossin <benno.lossin@proton.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. The default implementaiton should therefore call `kernel::build_error`, thus preventing calls to this function at compile time:
On 21.10.23 14:45, Alice Ryhl wrote: > On 10/19/23 19:15, 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@proton.me> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > There is a minor nit below, and in reviews sent by others, but overall > this looks fine to me. > >> /// # Examples >> /// >> /// ```ignore >> +/// # use kernel::error::VTABLE_DEFAULT_ERROR; >> /// use kernel::prelude::*; > I probably wouldn't hide the import of VTABLE_DEFAULT_ERROR from this > example. What do you guys think of putting the const it in the prelude?
Benno Lossin <benno.lossin@proton.me> writes: > On 20.10.23 11:06, Andreas Hindborg (Samsung) wrote: >> Benno Lossin <benno.lossin@proton.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. > The default implementaiton should > therefore call `kernel::build_error`, thus preventing calls to this > function at compile time: Otherwise I think it is good 👍 BR Andreas
On 23.10.23 10:30, Andreas Hindborg (Samsung) wrote: > > Benno Lossin <benno.lossin@proton.me> writes: > >> On 20.10.23 11:06, Andreas Hindborg (Samsung) wrote: >>> Benno Lossin <benno.lossin@proton.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.
On Thu, 19 Oct 2023 17:15:53 +0000 Benno Lossin <benno.lossin@proton.me> 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@proton.me> > --- > v1 -> v2: > - removed imperative mode in the paragraph describing optional > functions. > > rust/kernel/error.rs | 4 ++++ > rust/macros/lib.rs | 32 ++++++++++++++++++++++++-------- > 2 files changed, 28 insertions(+), 8 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..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: > +/// > +/// ```compile_fail > +/// # use kernel::error::VTABLE_DEFAULT_ERROR; > +/// kernel::build_error(VTABLE_DEFAULT_ERROR) Note that `build_error` function is considered impl detail and is hidden. This should use the macro version instead: kernel::build_error!(VTABLE_DEFAULT_ERROR) Actually, the string here provides little use other than documentation, since the string provided to build_error is only visible in const eval, so this you might just omit that and write kernel::build_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 function are required. > /// > /// # Examples > /// > /// ```ignore > +/// # use kernel::error::VTABLE_DEFAULT_ERROR; > /// use kernel::prelude::*; > /// > /// // Declares a `#[vtable]` trait > /// #[vtable] > -/// pub trait Operations: Send + Sync + Sized { > +/// pub trait Operations { > /// 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 +139,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: a7135d10754760f0c038497b44c2c2f2b0fb5651
On 24.10.23 13:24, Gary Guo wrote: > On Thu, 19 Oct 2023 17:15:53 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: [...] >> -/// 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: >> +/// >> +/// ```compile_fail >> +/// # use kernel::error::VTABLE_DEFAULT_ERROR; >> +/// kernel::build_error(VTABLE_DEFAULT_ERROR) > > Note that `build_error` function is considered impl detail and is > hidden. I see, we should mention that in the docs on `build_error`. > This should use the macro version instead: > > kernel::build_error!(VTABLE_DEFAULT_ERROR) Is there a reason that it is a macro? Why is it re-exported in the kernel crate? The macro could just use `::bulid_error::build_error()`. > Actually, the string here provides little use other than documentation, Sure, but that is the whole purpose of this patch. > since the string provided to build_error is only visible in const eval, > so this you might just omit that and write > > kernel::build_error!() Note that it is also useful for people who read the code, as they can search for the constant and understand why it is a build error.
On Mon, Oct 23, 2023 at 9:01 AM Benno Lossin <benno.lossin@proton.me> wrote: > > On 21.10.23 14:45, Alice Ryhl wrote: > > On 10/19/23 19:15, 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@proton.me> > > > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > > > There is a minor nit below, and in reviews sent by others, but overall > > this looks fine to me. > > > >> /// # Examples > >> /// > >> /// ```ignore > >> +/// # use kernel::error::VTABLE_DEFAULT_ERROR; > >> /// use kernel::prelude::*; > > I probably wouldn't hide the import of VTABLE_DEFAULT_ERROR from this > > example. > > What do you guys think of putting the const it in the prelude? I think it's fine to just import it. Alice
On Tue, 24 Oct 2023 14:43:30 +0000 Benno Lossin <benno.lossin@proton.me> wrote: > On 24.10.23 13:24, Gary Guo wrote: > > On Thu, 19 Oct 2023 17:15:53 +0000 > > Benno Lossin <benno.lossin@proton.me> wrote: > > [...] > > >> -/// 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: > >> +/// > >> +/// ```compile_fail > >> +/// # use kernel::error::VTABLE_DEFAULT_ERROR; > >> +/// kernel::build_error(VTABLE_DEFAULT_ERROR) > > > > Note that `build_error` function is considered impl detail and is > > hidden. > > I see, we should mention that in the docs on `build_error`. Well, it's marked as `#[doc(hidden)]`... > > > This should use the macro version instead: > > > > kernel::build_error!(VTABLE_DEFAULT_ERROR) > > Is there a reason that it is a macro? Why is it re-exported in the > kernel crate? The macro could just use `::bulid_error::build_error()`. The original intention is to allow format strings, but Rust const panic is not powerful enough to support it at the moment. Macro allows higher flexibility. It's re-exported so the macro can reference them (note that downstream crates can't reference build_error directly). Perhaps I should put it inside __private_reexport or something instead.
On 25.10.23 21:14, Gary Guo wrote: > On Tue, 24 Oct 2023 14:43:30 +0000 > Benno Lossin <benno.lossin@proton.me> wrote: > >> On 24.10.23 13:24, Gary Guo wrote: >>> On Thu, 19 Oct 2023 17:15:53 +0000 >>> Benno Lossin <benno.lossin@proton.me> wrote: >> >> [...] >> >>>> -/// 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: >>>> +/// >>>> +/// ```compile_fail >>>> +/// # use kernel::error::VTABLE_DEFAULT_ERROR; >>>> +/// kernel::build_error(VTABLE_DEFAULT_ERROR) >>> >>> Note that `build_error` function is considered impl detail and is >>> hidden. >> >> I see, we should mention that in the docs on `build_error`. > > Well, it's marked as `#[doc(hidden)]`... Yes, but I did not even build the docs, but read it directly inside of the `build_error` crate and thus I did not see the `#[doc(hidden)]`. >>> This should use the macro version instead: >>> >>> kernel::build_error!(VTABLE_DEFAULT_ERROR) >> >> Is there a reason that it is a macro? Why is it re-exported in the >> kernel crate? The macro could just use `::bulid_error::build_error()`. > > The original intention is to allow format strings, but Rust const > panic is not powerful enough to support it at the moment. Macro > allows higher flexibility. That is what I thought. But should we then not always require a string literal? So kernel::build_error!("{}", VTABLE_DEFAULT_ERROR) > It's re-exported so the macro can reference them (note that downstream > crates can't reference build_error directly). Perhaps I should put it > inside __private_reexport or something instead. I see, I did not know that they cannot reference build error directly. Is that some limitation of the build system? If it is possible to not re-export it, I think that would be better, otherwise just put it in `__private`.
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..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: +/// +/// ```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 function are required. /// /// # Examples /// /// ```ignore +/// # use kernel::error::VTABLE_DEFAULT_ERROR; /// use kernel::prelude::*; /// /// // Declares a `#[vtable]` trait /// #[vtable] -/// pub trait Operations: Send + Sync + Sized { +/// pub trait Operations { /// 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 +139,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)