Message-ID: <8734g2w6ka.fsf@kernel.org>
Date: Tue, 25 Feb 2025 13:29:57 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.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>,  "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 v4 2/4] rust: configfs: introduce rust support for configfs

"Daniel Almeida" <daniel.almeida@...labora.com> writes:

[...]

>>>> +
>>>> +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,
>>>
>>> Is it me or this can be simply &CStr? `config_item_set_name` either copies into an internal
>>> buffer or allocates, so I see no reason to pass an owned type here.
>>
>> This function returns an initializer that would be bound by the lifetime
>> of the reference you pass in. That in turn has a very negative effect on
>> the ergonomics of the function, as it would limit the places you can
>> call it and still satisfy lifetime requirements.
>>
>> We could pass in a borrow and create an owned instance from the borrow,
>> then move the owned value into the initializer. But I think taking an
>> owned parameter in the first place is better.
>
>
> Do you mean that the CString is used to guarantee that the string is alive when the initializer
> actually runs?

Exactly.

>
> If so, I did not consider that. Please disregard this comment then.

OK.

>
>>
>> [...]
>>
>>>> +
>>>> +/// # Safety
>>>> +///
>>>> +/// `this` must be a valid pointer.
>>>> +///
>>>> +/// If `this` does not represent the root group of a `configfs` subsystem,
>>>> +/// `this` must be a pointer to a `bindings::config_group` embedded in a
>>>> +/// `Group<Parent>`.
>>>> +///
>>>> +/// Otherwise, `this` must be a pointer to a `bindings::config_group` that
>>>> +/// is embedded in a `bindings::configfs_subsystem` that is embedded in a
>>>> +/// `Subsystem<Parent>`.
>>>> +unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a Parent {
>>>> +    // 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>`.
>>>
>>> This phrase is confusing.
>>
>> I am not sure how to rephrase it to be less confusing. The pointer is
>> the thing returned from `make_group`. `make_group` would only return a
>> pointer into a `Group<Parent>`.
>
> The problem is this: "that we passed as a return value in from”, to pass something as a return value
> is already hard to parse, and when you reach the “in from” part, it becomes even harder.
>
> Just say a variation of what you said above, that is perfectly understandable.
>
> What about:
>
> ```
>
> `this` is a pointer to a `bindings::config_group` that was returned from a call to `make_group`. The pointer is known
> to be embedded within a `Group<Parent>`.
>
> ```

How is this:

        // SAFETY: By C API contact,`this` was returned from a call to
        // `make_group`. The pointer is known to be embedded within a
        // `Group<Parent>`.

[...]

>>>> +
>>>> +/// A `configfs` attribute.
>>>> +///
>>>> +/// An attribute appears as a file in configfs, inside a folder that represent
>>>> +/// the group that the attribute belongs to.
>>>> +#[repr(transparent)]
>>>> +pub struct Attribute<const ID: u64, O, Data> {
>>>
>>> The first thing I thought here is “what is O?!"
>>
>> Would you prefer a rename to "Operations"? I was told to not add trait
>> bounds on the struct, because it is not necessary.
>
> I’d prefer Operations, yes.
>
>>
>>>
>>>> +    attribute: Opaque<bindings::configfs_attribute>,
>>>> +    _p: PhantomData<(O, Data)>,
>>>> +}
>>>> +
>>>> +// SAFETY: We do not provide any operations on `Attribute`.
>>>> +unsafe impl<const ID: u64, O, Data> Sync for Attribute<ID, O, Data> {}
>>>> +
>>>> +// SAFETY: Ownership of `Attribute` can safely be transferred to other threads.
>>>> +unsafe impl<const ID: u64, O, Data> Send for Attribute<ID, O, Data> {}
>>>> +
>>>> +impl<const ID: u64, O, Data> Attribute<ID, O, Data>
>>>> +where
>>>> +    O: AttributeOperations<ID, Data = Data>,
>>>
>>> I recommend renaming “O" into something that denotes this bound better.
>>>
>>> It can be terse as appropriate, but simply “O” is a bit *too terse* .
>>
>> Right, I agree. However, other reviewers have argued negatively about
>> using 4 letters for the "Data" bound, since generic type parameters are
>> often just single capital letters.
>
> This is a convention, that is all. As a person trying to make sense of this code, Data was *much*
> more informative than T or U, or etc. Same for `Parent`.
>
> If you see things like:
>
> ```
> impl<Data> Subsystem<Data>
> ```
>
> You already know it’s a type parameter. If you click on Data, assuming LSP support, it will also tell
> you that.
>
> This code is already a bit hard to follow as is, let’s make sure that the types actually aid in its
> comprehension and not the other way around, please.

I would agree. I believe Benno was arguing that it is difficult to see
what identifiers are generic type parameters when they are words rather
than letters.

[...]

>>>> +/// 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]>,
>>>
>>> For the sake of maintainability, can you provide some docs on this type?
>>>
>>> For example, what is the c_void here?
>>
>> As per the docstring above, is a list of `Attribute`. I can expand it a bit:
>>
>> /// 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.
>> +///
>> +/// # Note
>> +///
>> +/// This type is constructed statically at compile time and is by the
>> +/// [`kernel::configfs_attrs`] macro.
>> #[repr(transparent)]
>> pub struct AttributeList<const N: usize, Data>(
>> +    /// Null terminated Array of pointers to `Attribute`. The type is `c_void`
>> +    /// to conform to the C API.
>
> Yes this is much better :)

Let's go with that then.

[...]

>>>> +                $(
>>>> +                    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 >]
>>>> +                            );
>>>> +                )?
>>>> +
>>>> +                & [< $data:upper _TPE >]
>>>> +            }
>>>> +        }
>>>> +    };
>>>> +
>>>> +}
>>>
>>> Andreas, just a suggestion for the sake of maintainability, can you add some docs to this macro?
>>>
>>> I think you’ll agree that there is a *lot* going on here.
>>>
>>> In fact, this patch is a bit complex, so kudos on making it work in a very simple way for the callers.
>>
>> I could write a small wall of text and do some step by step expansion of
>> the macro. But I would rather not, since we are soon (TM) going to have
>> `syn` and `quote`, and all this horror will go away.
>
> Ok, your call.
>
>>
>> One way to help parsing this mess, is using the "expand macro" feature
>> of `rust-analyzer` in an editor and looking at the expanded code.
>
> I wonder if that can’t be pasted inline with the docs for a trivial use of the macro?
>
> I will take the verbosity *any day* over trying to figure out what is going on
> manually here.
>
> Or you can wait for syn and quote, as you said, that’s OK.
>
> By the way, with the somewhat limited support for rust-analyzer in the kernel,
> I wonder whether that is in fact possible. Things are much more restricted for
> non-Cargo projects.

It is working for me at least. I build the rust-project.json with `make
rust-analyzer`. I build out of tree, so I have to move the file to the
source tree manually. No additional steps required.


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists