Joel Fernandes
2025-Nov-26 20:14 UTC
[PATCH v2 3/3] rust: clist: Add typed iteration with FromListHead trait
> On Nov 25, 2025, at 8:03?PM, Alexandre Courbot <acourbot at 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.> > And if Clist ends up just being a provider for a ClistIterator, you > might just as well return a ClistIterator from the beginning. Anyway, > collapsing the two types into one can be done after the design matures > if it turns out to be the right thing to do, so feel free to keep both > for now.I prefer to generate the iterator separately as a second step in case we have to extend it to do something other than iteration later, makes it more future ready and better separation of concerns IMO, a list can be more than just iterating, and as you said, we can collapse it later if needed. Thanks.
Alexandre Courbot
2025-Nov-27 03:43 UTC
[PATCH v2 3/3] rust: clist: Add typed iteration with FromListHead trait
On Thu Nov 27, 2025 at 5:14 AM JST, Joel Fernandes wrote:> > >> On Nov 25, 2025, at 8:03?PM, Alexandre Courbot <acourbot at 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.