Re: [PATCH v2 3/3] rust: clist: Add typed iteration with FromListHead trait

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Nov 27, 2025 at 5:14 AM JST, Joel Fernandes wrote:
>
>
>> On Nov 25, 2025, at 8:03 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> 
>> On Wed Nov 26, 2025 at 8:29 AM JST, Joel Fernandes wrote:
>>> Hi Alex,
>>> 
>>> On 11/24/2025 2:01 AM, Alexandre Courbot wrote:
>>>>> ///
>>>>> /// # Invariants
>>>>> ///
>>>>> @@ -69,6 +156,15 @@ pub fn iter_heads(&self) -> ClistHeadIter<'_> {
>>>>>             head: &self.0,
>>>>>         }
>>>>>     }
>>>>> +
>>>>> +    /// Create a high-level iterator over typed items.
>>>>> +    #[inline]
>>>>> +    pub fn iter<L: ClistLink>(&self) -> ClistIter<'_, L> {
>>>>> +        ClistIter {
>>>>> +            head_iter: self.iter_heads(),
>>>>> +            _phantom: PhantomData,
>>>>> +        }
>>>>> +    }
>>>> This looks very dangerous, as it gives any caller the freedom to specify
>>>> the type they want to upcast the `Clist` to, without using unsafe code.
>>>> One could easily invoke this with the wrong type and get no build error
>>>> or warning whatsoever.
>>>> 
>>>> A safer version would have the `Clist` generic over the kind of
>>>> conversion that needs to be performed, using e.g. a closure:
>>>> 
>>>>  pub struct Clist<'a, T, C: Fn(*mut bindings::list_head) -> *mut T> {
>>>>      head: &'a ClistHead,
>>>>      conv: C,
>>>>  }
>>>> 
>>>> `from_raw` would also take the closure as argument, which forces the
>>>> creator of the list to both specify what that list is for, and use an
>>>> `unsafe` statement for unsafe code. Here is a dummy example:
>>>> 
>>>>    let head: bindings::list_head = ...;
>>>> 
>>>>    // SAFETY: list_head always corresponds to the `list` member of
>>>>    // `type_embedding_list_head`.
>>>>    let conv = |head: *mut bindings::list_head| unsafe {
>>>>        crate::container_of!(head, type_embedding_list_head, list)
>>>>    };
>>>> 
>>>>    // SAFETY: ...
>>>>    unsafe { Clist::from_raw(head, conv) }
>>>> 
>>>> Then `conv` would be passed down to the `ClistIter` so it can return
>>>> references to the correct type.
>>>> 
>>>> By doing so you can remove the `ClinkList` and `FromListHead` traits,
>>>> the `impl_from_list_head` and `clist_iterate` macros, as well as the
>>>> hidden ad-hoc types these create. And importantly, all unsafe code must
>>>> be explicitly specified in an `unsafe` block, nothing is hidden by
>>>> macros.
>>>> 
>>>> This approach works better imho because each `list_head` is unique in
>>>> how it has to be iterated: there is no benefit in implementing things
>>>> using types and traits that will only ever be used in a single place
>>>> anyway. And if there was, we could always create a newtype for that.
>>> 
>>> I agree with your safety concerns, indeed it is possible without any safety
>>> comments to build iterators yielding objects of random type. I think the conv
>>> function is a good idea and with the addition of unsafe blocks within the conv.
>>> 
>>> One thing I am concerned is with the user interface. I would like to keep the
>>> user interface as simple as possible. I am hoping that with implementing your
>>> idea here on this with the closure, we can still keep it simple, perhaps getting
>>> the assistance of macros. I will give it a try.
>> 
>> We should be able to build more convenient interfaces on top of this
>> closure-based design (hopefully without the help of macros).
>> 
>> But first, one needs to recognize that this Clist interface is not your
>> regular, easy-to-use Rust interface - it is designed for specific cases
>> where we need to interact with C code and do unsafe things anyway.
>> 
>> Still, the most common (maybe even the only?) conversion pattern will be
>> "substract an offset from the address of this list_head and cast to this
>> type". Instead of expressing this through a closure using
>> `container_of`, maybe we can have a dedicated constructor for these
>> cases:
>> 
>>  pub unsafe from_raw_and_offset<const LIST_OFFSET: usize>(ptr: *mut bindings::list_head) ->  Clist<'a, T, ...>
>> 
>> `LIST_OFFSET` could be specified by callers using the `offset_of` macro.
>> This method would then call the more generic `from_raw` constructor,
>> passing the right closure. And with that you have a more convenient
>> interface. :)
>
> Great! This makes it easy to use. I will do it this way then - I am assuming everyone is ok baking in this kind of usecase assumed (subtraction of offset). If anyone is not, please raise your concern. 
>
>> 
>>> 
>>>> Also as I suspected in v1 `Clist` appears to do very little apart from
>>>> providing an iterator, so I'm more convinced that the front type for
>>>> this should be `ClistHead`.
>>> 
>>> This part I don't agree with. I prefer to keep it as `Clist` which wraps a
>>> sentinel list head. A random `ClistHead` is not necessarily a sentinel.
>> 
>> I expressed myself poorly - what I meant of that `ClistHead` should be
>> the only type you need for the low-level iteration (which should not be
>> public).
>
> For low level iteration it is already via that type. There are 2 iterators. The higher level uses the lower level one. I could make it even simpler and collapse bother iterators into one - that yields the final type T. 

I think the current 2 iterators design is elegant: the lower-level one
taking care of going through the list, and the higher-level one building
on top of that and adding upcasting. Maybe the lower-level one can be
made private, but I'd keep it in any case.