Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Benno Lossin]

On 17.02.25 13:20, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>> On 17.02.25 12:08, Andreas Hindborg wrote:
>>>>> +
>>>>> +/// 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.
>>
>> Makes sense. I'm not opposed to it, but I am a bit cautious, because one
>> disadvantage with using multi-character names for generics is that one
>> cannot easily see if a type is a generic or not. Maybe that is not as
>> important as I think it could be, but to me it seems useful.
> 
> If you use an editor with semantic highlighting, you can style the
> generic identifiers. I am currently trying out Helix, and that is
> unfortunately on of the features it is missing. Can't have it all I
> guess.

That is true, but there are a lot of places where Rust code is put that
aren't my editor (git diffs/commit messages, mails, lore.kernel.org,
github) and there it'll become more difficult to read (also people might
not have their editor configured to highlight them).

So I think we should at least consider it more.

>>>>> +                    // 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.
>>
>> Where is this static check?
> 
> In `Attribute::add`:
> 
>         if I >= N - 1 {
>             kernel::build_error!("Invalid attribute index");
>         }

Ahh I see, would be also nice to have a comment there explaining why the
check is `>= N - 1`.

---
Cheers,
Benno