> On Nov 25, 2025, at 8:03 PM, Alexandre Courbot <[email protected]> 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.
