[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87msbw1s9e.fsf@kernel.org>
Date: Thu, 01 May 2025 13:31:57 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com>
Cc: "Danilo Krummrich" <dakr@...nel.org>, "Miguel Ojeda"
<ojeda@...nel.org>, "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>, "Benno Lossin" <benno.lossin@...ton.me>,
"Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
"Joel Becker" <jlbec@...lplan.org>, "Peter Zijlstra"
<peterz@...radead.org>, "Ingo Molnar" <mingo@...hat.com>, "Will Deacon"
<will@...nel.org>, "Waiman Long" <longman@...hat.com>, "Fiona Behrens"
<me@...enk.dev>, "Charalampos Mitrodimas" <charmitro@...teo.net>,
"Daniel Almeida" <daniel.almeida@...labora.com>, "Breno Leitao"
<leitao@...ian.org>, <rust-for-linux@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 1/3] rust: configfs: introduce rust support for configfs
"Miguel Ojeda" <miguel.ojeda.sandonis@...il.com> writes:
> On Thu, May 1, 2025 at 12:15 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>>
>> ---
>>
>
> Spurious newlines.
The one just before the cut?
>
>> This patch is a direct dependency for `rnull`, the rust null block driver.
>> ---
>
> By the way, you don't need this `---`.
b4 adds it 🤷
>
>> +//! `configfs` interface.
>
> I don't know if configfs is supposed to be written in code spans, but
> I appreciate you are trying to be throughout in your Markdown use ;)
> It may be easier to read to not have it in code spans, since we have
> many already and it is not code anyway.
OK
>
> By the way, you may want to mention somehow the title they use in
> their docs: "Userspace-driven Kernel Object Configuration".
Will do.
>
>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>
> Files are, though, like the C header below, so: [`rust_configfs.rs`]
OK
>
>> +/// with configfs, embed a field of this type into your kernel module struct.
>
> Either with or without a code span, i.e. being consistent is best.
I am! Consistently inconsistent. Very much so in this series. Will fix.
>
>> + /// Return the address of the `bindings::config_group` embedded in `Self`.
>
> I think you may be able to use intra-doc links for [`Self`].
Thanks. Would be nice with a lint for missed intra-doc links.
>
>> + let c_group: *mut bindings::config_group =
>> + // SAFETY: By function safety requirements, `item` is embedded in a
>> + // `config_group`.
>> + unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
>
> It doesn't work to put the safety comment on top? (We had issues
> similar to this in the past, so if it is intentional, that is fine).
Clippy gets mad if we move it up. Because rustfmt wants the unsafe block
to a new line:
warning: unsafe block missing a safety comment
--> rust/kernel/configfs.rs:557:13
|
557 | unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a safety comment on the preceding line
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
= note: requested on the command line with `-W clippy::undocumented-unsafe-blocks`
>
>> +/// This type is constructed statically at compile time and is by the
>> +/// [`kernel::configfs_attrs`] macro.
>
> Sentence is missing something. Also, we never used `# Note` yet, but I
> guess it is fine.
Thanks, rephrased:
# Note
Instances of this type are constructed statically at compile by the
[`kernel::configfs_attrs`] macro.
>
>> + /// Null terminated Array of pointers to `Attribute`. The type is `c_void`
>
> Intar-doc link(s)?
>
>> + // We need a space at the end of our list for a null terminator.
>> + if I >= N - 1 {
>> + kernel::build_error!("Invalid attribute index");
>> + }
>
> Would the following work instead?
>
> const { assert!(I < N - 1, "Invalid attribute index") };
>
> (Please double-check it actually catches the cases you need)
The reason I choose build_error is that if this should somehow end up
being evaluated in non-const context at some point, I want the build to
fail if the condition is not true. I don't think I get that with assert?
Best regards,
Andreas Hindborg
Powered by blists - more mailing lists