| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87h64su8ux.fsf@kernel.org>
Date: Mon, 17 Feb 2025 12:08:22 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
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>, "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor
Gross" <tmgross@...ch.edu>, "Joel Becker" <jlbec@...lplan.org>,
"Christoph Hellwig" <hch@....de>, "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>,
<rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs
"Benno Lossin" <benno.lossin@...ton.me> writes:
> On 07.02.25 15:41, Andreas Hindborg wrote:
>> This patch adds a rust API for configfs, thus allowing rust modules to use
>> configfs for configuration. The implementation is a shim on top of the C
>> configfs implementation allowing safe use of the C infrastructure from
>> rust.
>>
>> The patch enables the `const_mut_refs` feature on compilers before rustc
>> 1.83. The feature was stabilized in rustc 1.83 and is not required to be
>> explicitly enabled on later versions.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>>
>> ---
>>
>> This patch is a direct dependency for `rnull`, the rust null block driver.
>> ---
>> rust/bindings/bindings_helper.h | 1 +
>> rust/helpers/mutex.c | 5 +
>> rust/kernel/configfs.rs | 860 ++++++++++++++++++++++++++++++++++++++++
>> rust/kernel/lib.rs | 2 +
>> samples/rust/Kconfig | 11 +
>> samples/rust/Makefile | 1 +
>> samples/rust/rust_configfs.rs | 186 +++++++++
>
> Can you move the sample into its own patch?
Yes.
[...]
>> diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
>> new file mode 100644
>> index 0000000000000..9d4b381b9df89
>> --- /dev/null
>> +++ b/rust/kernel/configfs.rs
>> @@ -0,0 +1,860 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! `configfs` interface.
>> +//!
>> +//! `configfs` is an in-memory pseudo file system for configuration of kernel
>> +//! modules. Please see the [C documentation] for details and intended use of
>> +//! `configfs`.
>> +//!
>> +//! This module does not support the following `configfs` features:
>> +//!
>> +//! - Items. All group children are groups.
>> +//! - Symlink support.
>> +//! - `disconnect_notify` hook.
>> +//! - Default groups.
>
> For lists like this, I usually end items except the last one with a
> comma instead of a period.
If that is the right way to do it, sure. It is actually funny that you
notice, because I searched for input on how to typeset such a list, and
some Microsoft typesetting site told me to do it like this when some
items are sentences [1]. I am not a native English speaker, and I have
no idea what the correct formatting is. Commas are not mentioned at the
resource I found.
[1] https://www.microsoft.com/en-us/microsoft-365-life-hacks/writing/punctuating-bullet-point-lists
>
>> +//!
>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>
> It could also be useful to just put the example directly here into the
> docs instead of/additionally to having it as a sample.
I don't think we should duplicate the code. As long as the link works, I
think having it separately is fine.
>
>> +//!
>> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
>> +//!
>> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
>> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
>> +
>> +use crate::alloc::flags;
>> +use crate::container_of;
>> +use crate::page::PAGE_SIZE;
>> +use crate::prelude::*;
>> +use crate::str::CString;
>> +use crate::sync::Arc;
>> +use crate::types::ForeignOwnable;
>> +use crate::types::Opaque;
>> +use core::cell::UnsafeCell;
>> +use core::marker::PhantomData;
>> +use core::ptr::addr_of;
>> +use core::ptr::addr_of_mut;
>
> I usually would import this like so:
>
> use crate::{
> alloc::flags,
> container_of,
> page::PAGE_SIZE,
> prelude::*,
> str::CString,
> sync::Arc,
> types::{ForeignOwnable, Opaque},
> };
> use core::{
> cell::UnsafeCell,
> marker::PhantomData,
> ptr::{addr_of, addr_of_mut},
> };
>
> To me this is more readable.
I disagree with that. I don't think what you suggest is easier to read,
and it is much more difficult to work with when rebasing and merging
things. This was discussed elsewhere in the past without reaching a
conclusion. I think we should come to a consensus on what style we
should adopt for the imports.
>
>> +
>> +/// A `configfs` subsystem.
>> +///
>> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
>> +/// with configfs, embed a field of this type into your kernel module struct.
>> +#[pin_data(PinnedDrop)]
>> +pub struct Subsystem<Data> {
>
> Usually, we don't have multi-character generics, any specific reason
> that you chose `Data` here over `T` or `D`?
Yes, I find it more descriptive. The patch set went through quite a bit
of evolution, and the generics got a bit complicated in earlier
iterations, which necessitated more descriptive generic type parameter
names. It's not so bad in this version after I restricted the pointer
type to just `Arc`, but I still think that using a word rather a single
letter makes the code easier to comprehend at first pass.
Also, using a word is allowed as per the API guideline document [2]:
> concise UpperCamelCase, usually single uppercase letter: T
https://rust-lang.github.io/api-guidelines/naming.html
>
>> + #[pin]
>> + subsystem: Opaque<bindings::configfs_subsystem>,
>> + #[pin]
>> + data: Data,
>> +}
>> +
>> +// SAFETY: We do not provide any operations on `Subsystem`.
>> +unsafe impl<Data> Sync for Subsystem<Data> {}
>> +
>> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
>> +unsafe impl<Data> Send for Subsystem<Data> {}
>> +
>> +impl<Data> Subsystem<Data> {
>> + /// Create an initializer for a [`Subsystem`].
>> + ///
>> + /// The subsystem will appear in configfs as a directory name given by
>> + /// `name`. The attributes available in directory are specified by
>> + /// `item_type`.
>> + pub fn new(
>> + name: &'static CStr,
>> + item_type: &'static ItemType<Subsystem<Data>, Data>,
>> + data: impl PinInit<Data, Error>,
>> + ) -> impl PinInit<Self, Error> {
>> + try_pin_init!(Self {
>> + subsystem <- kernel::init::zeroed().chain(
>> + |place: &mut Opaque<bindings::configfs_subsystem>| {
>> + // SAFETY: All of `place` is valid for write.
>> + unsafe {
>> + addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
>> + .write(name.as_ptr().cast_mut().cast())
>> + };
>> + // SAFETY: All of `place` is valid for write.
>> + unsafe {
>> + addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
>> + .write(item_type.as_ptr())
>> + };
>> + // SAFETY: We initialized the required fields of `place.group` above.
>> + unsafe { bindings::config_group_init(&mut (*place.get()).su_group) };
>> + // SAFETY: `place.su_mutex` is valid for use as a mutex.
>> + unsafe { bindings::__mutex_init(
>> + &mut (*place.get()).su_mutex,
>> + kernel::optional_name!().as_char_ptr(),
>> + kernel::static_lock_class!().as_ptr())
>
> Formatting for this code is weird.
>
> (since this is inside of the `try_pin_init!` macro, rustfmt doesn't
> format it, since `<-` isn't part of rust syntax, so it doesn't know what
> to do. I usually fix this by replacing all `<-` with `:`, format and
> then change things back)
Such is the perils of macros. I'll try to go over it again. Perhaps we
could make `rustfmt` understand `<-`?
>
> Also, is there no function in C that does all of this initialization for
> you?
I might be able to do a little better. There is a C function that takes
care of initialization of `ci_name` and `ci_type` as well. I can't
recall if there was a particular reason for not using it, but I'll
check.
We have to initialize the mutex explicitly. I think the reason for not
doing that implicitly C side is to allow it to be statically
initialized.
[...]
>> +
>> +impl<Data> Group<Data> {
>> + /// Create an initializer for a new group.
>> + ///
>> + /// When instantiated, the group will appear as a directory with the name
>> + /// given by `name` and it will contain attributes specified by `item_type`.
>> + pub fn new(
>> + name: CString,
>> + item_type: &'static ItemType<Group<Data>, Data>,
>> + data: impl PinInit<Data, Error>,
>> + ) -> impl PinInit<Self, Error> {
>> + try_pin_init!(Self {
>> + group <- kernel::init::zeroed().chain(|v: &mut Opaque<bindings::config_group>| {
>> + let place = v.get();
>> + let name = name.as_bytes_with_nul().as_ptr();
>> + // SAFETY: It is safe to initialize a group once it has been zeroed.
>> + unsafe {
>> + bindings::config_group_init_type_name(place, name as _, item_type.as_ptr())
>
> Can you replace the `as _` cast with a `.cast()`?
π
[...]
>> +unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a Parent {
>> + // TODO
>
> Missed this todo?
Thanks. It was referring to a missing safety comment that was later put
in place.
>
>> + // SAFETY: `this` is a valid pointer.
>> + let is_root = unsafe { (*this).cg_subsys.is_null() };
>> +
>> + if !is_root {
>> + // SAFETY: By C API contact, `this` is a pointer to a
>> + // `bindings::config_group` that we passed as a return value in from
>> + // `make_group`. Such a pointer is embedded within a `Group<Parent>`.
>> + unsafe { &(*Group::<Parent>::container_of(this)).data }
>> + } else {
>> + // SAFETY: By C API contract, `this` is a pointer to the
>> + // `bindings::config_group` field within a `Subsystem<Parent>`.
>> + unsafe { &(*Subsystem::container_of(this)).data }
>> + }
>> +}
>> +
>> +struct GroupOperationsVTable<Parent, Child>(PhantomData<(Parent, Child)>)
>
> Generic names?
I prefer descriptive names. I don't really see the point of replacing
with `P,C`. It would not be better.
>
>> +where
>> + Parent: GroupOperations<Child = Child>;
>
> No need to put this where bound on the struct definition (it is only
> needed if the struct impls `Drop`).
Right.
[...]
>> +
>> +/// Operations implemented by `configfs` groups that can create subgroups.
>> +///
>> +/// Implement this trait on structs that embed a [`Subsystem`] or a [`Group`].
>> +#[vtable]
>> +pub trait GroupOperations {
>> + /// The parent data object type.
>> + ///
>> + /// The implementer of this trait is this kind of data object. Should be set
>> + /// to `Self`.
>> + type Parent;
>
> If it should be set to `Self`, why does this even exist? If there are
> cases where it isn't supposed to be `Self`, it would be good to put them
> into the docs.
Good point. I'll remove the type and use `&self` at relevant places
instead.
>
>> +
>> + /// The child data object type.
>> + ///
>> + /// This group will create subgroups (subdirectories) backed by this kind of
>> + /// object.
>> + type Child: 'static;
>> +
>> + /// The kernel will call this method in response to `mkdir(2)` in the
>> + /// directory representing `this`.
>
> This doesn't really read like a first line description of this function,
> how about putting "Creates a new subgroup." as the first line?
Good call.
>
>> + ///
>> + /// To accept the request to create a group, implementations should
>> + /// instantiate a `CHLD` and return a `CPTR` to it. To prevent creation,
>
> Is there a typo in `CHLD`? What do you mean by "return a `CPTR` to it"?
Sorry, it's from an older iteration of the patch set. I'll update the
sentence. Looking forward to the day where the compiler can use AI to
type check the docs :)
>
>> + /// return a suitable error.
>> + fn make_group(
>> + this: &Self::Parent,
>> + name: &CStr,
>> + ) -> Result<impl PinInit<Group<Self::Child>, Error>>;
>> +
>> + /// The kernel will call this method before the directory representing
>> + /// `_child` is removed from `configfs`.
>
> Same thing about the one-line description, how about (with the name
> changed below): "Prepares the given group for removal from configfs.".
π
>
>> + ///
>> + /// Implementations can use this method to do house keeping before
>> + /// `configfs` drops its reference to `Child`.
>> + fn drop_item(
>
> `drop` doesn't really fit here, I think something like `unlink_item`
> fits better, since the child isn't actually dropped after this function
> returns.
Yea, I know. But the function is called `drop_item` on the C side of
things. Usually we keep the C names.
>
>> + _this: &Self::Parent,
>> + _child: <Arc<Group<Self::Child>> as ForeignOwnable>::Borrowed<'_>,
>
> Just write ArcBorrow<'_, Group<Self::Child>> instead of the above?
Right. An earlier version of the patch set was generic over the pointer
type, allowing use of Box as well. The bounds became sort of wild, so I
figured limiting to `Arc` is probably fine.
>
>> + ) {
>> + kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
>> + }
>> +}
>> +
>> +/// A `configfs` attribute.
>> +///
>> +/// An attribute appear as a file in configfs, inside a folder that represent
>
> Typo: appear -> appears
π
>
[...]
>> +/// Operations supported by an attribute.
>> +///
>> +/// Implement this trait on type and pass that type as generic parameter when
>> +/// creating an [`Attribute`]. The type carrying the implementation serve no
>> +/// purpose other than specifying the attribute operations.
>> +#[vtable]
>> +pub trait AttributeOperations<const ID: u64 = 0> {
>> + /// The type of the object that contains the field that is backing the
>> + /// attribute for this operation.
>> + type Data;
>> +
>> + /// This function is called by the kernel to read the value of an attribute.
>
> "Reads the value of an attribute.".
π
>
>> + ///
>> + /// Implementations should write the rendering of the attribute to `page`
>> + /// and return the number of bytes written.
>> + fn show(data: &Self::Data, page: &mut [u8; PAGE_SIZE]) -> Result<usize>;
>
> Why is this not named `read` or `load`? If the C equivalent is `show`,
> then it's fine, but otherwise I wouldn't understand why it's show/store
> as opposed to load/store or read/write.
Yes, also here naming is derived from the C counterpart. The vtable item
is called `show`.
>
>> +
>> + /// This function is called by the kernel to update the value of an attribute.
>
> Again first line doc here.
π
>
>> + ///
>> + /// Implementations should parse the value from `page` and update internal
>> + /// state to reflect the parsed value. Partial writes are not supported and
>> + /// implementations should expect the full page to arrive in one write
>> + /// operation.
>
> I don't understand what you're trying to say with the last sentence.
I will remove the comment, it does not make sense here. I picked it up
from C docs. It refers to how the function is exposed to user space.
Usually writes to files are processed in chunks, and file systems cannot
expect that all data written to a file will be passed in a single
function call.
>
>> + fn store(_data: &Self::Data, _page: &[u8]) -> Result {
>> + kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
>> + }
>> +}
>> +
>> +/// A list of attributes.
>> +///
>> +/// This type is used to construct a new [`ItemType`]. It represents a list of
>> +/// [`Attribute`] that will appear in the directory representing a [`Group`].
>> +/// Users should not directly instantiate this type, rather they should use the
>> +/// [`kernel::configfs_attrs`] macro to declare a static set of attributes for a
>> +/// group.
>> +#[repr(transparent)]
>> +pub struct AttributeList<const N: usize, Data>(
>> + UnsafeCell<[*mut kernel::ffi::c_void; N]>,
>> + PhantomData<Data>,
>> +);
>> +
>> +// SAFETY: Ownership of `AttributeList` can safely be transferred to other threads.
>> +unsafe impl<const N: usize, Data> Send for AttributeList<N, Data> {}
>> +
>> +// SAFETY: We do not provide any operations on `AttributeList` that need synchronization.
>> +unsafe impl<const N: usize, Data> Sync for AttributeList<N, Data> {}
>> +
>> +impl<const N: usize, Data> AttributeList<N, Data> {
>> + #[doc(hidden)]
>
> I normally put attributes after the documentation.
Ok π
>
>> + /// # Safety
>> + ///
>> + /// This function can only be called by expanding the `configfs_attrs`
>
> s/expanding//
Ok.
>
>> + /// macro.
>> + pub const unsafe fn new() -> Self {
>> + Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
>> + }
>> +
>> + #[doc(hidden)]
>> + /// # Safety
>> + ///
>> + /// This function can only be called by expanding the `configfs_attrs`
>
> s/expanding//
Ok.
>
>> + /// macro.
>> + pub const unsafe fn add<
>> + const I: usize,
>> + const ID: u64,
>> + O: AttributeOperations<ID, Data = Data>,
>> + >(
>> + &'static self,
>> + attribute: &'static Attribute<ID, O, Data>,
>> + ) {
>> + if I >= N - 1 {
>> + kernel::build_error!("Invalid attribute index");
>> + }
>> +
>> + // SAFETY: This function is only called through `configfs_attrs`. This
>
> s/through `configfs_attrs`/through the `configfs_attrs` macro/
Ok.
>
[...]
>> +impl_item_type!(Subsystem<Data>);
>> +impl_item_type!(Group<Data>);
>> +
>> +impl<Container, Data> ItemType<Container, Data> {
>> + fn as_ptr(&self) -> *const bindings::config_item_type {
>> + self.item_type.get()
>> + }
>> +}
>> +
>> +/// Define a list of configfs attributes statically.
>> +#[macro_export]
>> +macro_rules! configfs_attrs {
>
> I see you've joined the dark side of declarative macros!
>
> This seems like a prime candidate for replacing with a proc-macro when
> we have syn :)
Yes, I found myself wishing for `syn` very much while writing this.
>
>> + (
>> + container: $container:ty,
>> + data: $data:ty,
>> + attributes: [
>> + $($name:ident: $attr:literal,)*
>
> This syntax always requires a trailing comma. Most (IIRC all, but not
> 100% sure) Rust syntax allows you to omit it, so it would be odd if it
> were not the case here. You can have an optional trailing comma via:
>
> $($name:ident: $attr:literal),* $(,)?
>
> But as soon as you give the tokens off to the internals of the macro, I
> would recommend sticking to always having a trailing comma or no
> trailing comma.
Makes sense, I will fix that. Perhaps after the array square brackets as well.
>
>> + ],
>> + ) => {
>> + $crate::configfs_attrs!(
>> + count:
>> + @container($container),
>> + @data($data),
>> + @child(),
>> + @no_child(x),
>> + @attrs($($name $attr)*),
>> + @eat($($name $attr,)*),
>> + @assign(),
>> + @cnt(0usize),
>> + )
>> + };
>> + (
>> + container: $container:ty,
>> + data: $data:ty,
>> + child: $child:ty,
>> + attributes: [
>> + $($name:ident: $attr:literal,)*
>
> Ditto.
>
>> + ],
>> + ) => {
>> + $crate::configfs_attrs!(
>> + count:
>> + @container($container),
>> + @data($data),
>> + @child($child),
>> + @no_child(),
>> + @attrs($($name $attr)*),
>> + @eat($($name $attr,)*),
>> + @assign(),
>> + @cnt(0usize),
>> + )
>> + };
>> + (count:
>> + @container($container:ty),
>> + @data($data:ty),
>> + @child($($child:ty)?),
>> + @no_child($($no_child:ident)?),
>> + @attrs($($aname:ident $aattr:literal)*),
>> + @eat($name:ident $attr:literal, $($rname:ident $rattr:literal,)*),
>> + @assign($($assign:block)*),
>> + @cnt($cnt:expr),
>> + ) => {
>> + $crate::configfs_attrs!(
>> + count:
>> + @container($container),
>> + @data($data),
>> + @child($($child)?),
>> + @no_child($($no_child)?),
>> + @attrs($($aname $aattr)*),
>> + @eat($($rname $rattr,)*),
>> + @assign($($assign)* {
>> + const N: usize = $cnt;
>> + // SAFETY: We are expanding `configfs_attrs`.
>
> This safety comment is doing a lot of heavy lifting, since it is not at
> all obvious what the below unsafe function will resolve to... Seems also
> a hassle to put a full comment here explaining that
> `[< $data:upper _ATTRS >]` is defined by the macro below and that it is
> of type `AttributeList` etc... But maybe we should.
I mean, I can put a comment saying that this will expand to a call to `Attribute::add`?
>
>> + unsafe {
>> + $crate::macros::paste!( [< $data:upper _ATTRS >])
>> + .add::<N, $attr, _>(
>> + & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
>> + )
>> + };
>> + }),
>
> You can merge the two `paste!` invocations into one:
Is that better?
>
> @assign($($assign)* {
> const N: usize = $cnt;
> $crate::macros::paste! {
> // SAFETY: see above comment
> unsafe {
> [< $data:upper _ATTRS >].add::<N, $attr, _>(
> &[< $data:upper _ $name:upper _ATTR >]
> );
> }
> }
> }),
>
>> + @cnt(1usize + $cnt),
>> + )
>> + };
>> + (count:
>> + @container($container:ty),
>> + @data($data:ty),
>> + @child($($child:ty)?),
>> + @no_child($($no_child:ident)?),
>> + @attrs($($aname:ident $aattr:literal)*),
>> + @eat(),
>> + @assign($($assign:block)*),
>> + @cnt($cnt:expr),
>> + ) =>
>> + {
>> + $crate::configfs_attrs!(
>> + final:
>> + @container($container),
>> + @data($data),
>> + @child($($child)?),
>> + @no_child($($no_child)?),
>> + @attrs($($aname $aattr)*),
>> + @assign($($assign)*),
>> + @cnt($cnt),
>> + )
>> + };
>> + (final:
>> + @container($container:ty),
>> + @data($data:ty),
>> + @child($($child:ty)?),
>> + @no_child($($no_child:ident)?),
>> + @attrs($($name:ident $attr:literal)*),
>> + @assign($($assign:block)*),
>> + @cnt($cnt:expr),
>> + ) =>
>> + {
>> + {
>> + $(
>> + $crate::macros::paste!{
>
> Again you can coalesce all of the `paste!` invocations into a single one
> spanning the entire output of this macro branch.
Is that better? I actually tried to keep them smaller.
>
>> + // SAFETY: We are expanding `configfs_attrs`.
>> + static [< $data:upper _ $name:upper _ATTR >]:
>> + $crate::configfs::Attribute<$attr, $data, $data> =
>> + unsafe {
>> + $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
>> + };
>> + }
>> + )*
>> +
>> +
>> + const N: usize = $cnt + 1usize;
>
> Why do we need an additional copy? To have a zero entry at the end for C
> to know it's the end of the list? If so, a comment here would be very
> helpful.
Yes, we need space for a null terminator. I'll add a comment.
We actually have a static check to make sure that we not missing this.
>
>> + $crate::macros::paste!{
>> + // SAFETY: We are expanding `configfs_attrs`.
>> + static [< $data:upper _ATTRS >]:
>> + $crate::configfs::AttributeList<N, $data> =
>> + unsafe { $crate::configfs::AttributeList::new() };
>> + }
>> +
>> + $($assign)*
>> +
>> + $(
>> + $crate::macros::paste!{
>> + const [<$no_child:upper>]: bool = true;
>> + };
>> +
>> + $crate::macros::paste!{
>> + static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data> =
>> + $crate::configfs::ItemType::<$container, $data>::new::<N>(
>> + &THIS_MODULE, &[<$ data:upper _ATTRS >]
>> + );
>> + }
>> + )?
>> +
>> + $(
>> + $crate::macros::paste!{
>> + static [< $data:upper _TPE >]:
>> + $crate::configfs::ItemType<$container, $data> =
>> + $crate::configfs::ItemType::<$container, $data>::new_with_child_ctor::
>> + <N, $child>(
>> + &THIS_MODULE, &[<$ data:upper _ATTRS >]
>> + );
>> + }
>> + )?
>> +
>> + &$crate::macros::paste!( [< $data:upper _TPE >] )
>> + }
>> + };
>> +
>> +}
>
> I tested if multiple invocations of this macro can shadow each other and
> the answer is no. So wrapping a const with `{}` makes it inaccessible to
> the outside which is exactly what we need here.
> The macro looks quite good!
Well you can guess where the inspiration came from :)
>
[...]
>> diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs
>> new file mode 100644
>> index 0000000000000..fe896e66efb41
>> --- /dev/null
>> +++ b/samples/rust/rust_configfs.rs
>> @@ -0,0 +1,186 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Rust configfs sample.
>> +
>> +use kernel::alloc::flags;
>> +use kernel::c_str;
>> +use kernel::configfs;
>> +use kernel::configfs_attrs;
>> +use kernel::new_mutex;
>> +use kernel::prelude::*;
>> +use kernel::sync::Mutex;
>
> Would merge the imports here too (rust-analyzer has a code-action for
> that btw).
I prefer it like this.
>
>> +
>> +module! {
>> + type: RustConfigfs,
>> + name: "rust_configfs",
>> + author: "Rust for Linux Contributors",
>> + description: "Rust configfs sample",
>> + license: "GPL",
>> +}
>> +
>> +#[pin_data]
>> +struct RustConfigfs {
>> + #[pin]
>> + config: configfs::Subsystem<Configuration>,
>> +}
>> +
>> +#[pin_data]
>> +struct Configuration {
>> + message: &'static CStr,
>> + #[pin]
>> + bar: Mutex<(KBox<[u8; 4096]>, usize)>,
>> +}
>> +
>> +impl Configuration {
>> + fn new() -> impl PinInit<Self, Error> {
>> + try_pin_init!(Self {
>> + message: c_str!("Hello World\n"),
>> + bar <- new_mutex!((KBox::new([0;4096], flags::GFP_KERNEL)?,0)),
>
> s/;/; /
> s/,0/, 0/
π
Thanks for the detailed review!
Best regards,
Andreas Hindborg
Powered by blists - more mailing lists