On Thu Nov 27, 2025 at 5:14 AM JST, Joel Fernandes wrote: > > >> 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.
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.
