On Thu, Feb 19, 2026 at 09:35:31AM +0900, Alexandre Courbot wrote:
> I saw that v10 has been sent, but since the code is mostly identical
> this review should still apply (I was in the middle of writing it before
> going to sleep).
Thanks for the thorough review, Alex! All comments addressed below.
> > + * Helpers for C Circular doubly linked list implementation.
>
> nit: "Circular" doesn't need a capital "C".
Fixed.
> > +//! # // SAFETY: pointers are to allocated test objects with a
> > list_head field.
> > +//! # unsafe {
> > +//! # (*ptr).value = i as i32 * 10;
> > +//! # // &raw mut computes address of link directly as link is
> > uninitialized.
> > +//! # bindings::INIT_LIST_HEAD(&raw mut (*ptr).link);
> > +//! # bindings::list_add_tail(&mut (*ptr).link, head);
>
> Is the `INIT_LIST_HEAD` line needed? `list_add_tail` will immediately
> overwrite the initial values of `ptr.link`.
Good point - list_add_tail does overwrite the link fields. However I'm
keeping the INIT_LIST_HEAD call because the doctest is meant to show
the full pattern of working with list_head from Rust, including proper
initialization. Also, skipping it would mean calling list_add_tail on
an uninitialized list_head, which while it works in practice (since
list_add_tail overwrites both fields), feels wrong to demonstrate in
an example.
> > +//! let list = unsafe { clist_create!(head, Item, SampleItemC, link) };
>
> `head` appears in the documentation for the first time here - it would
> help to make at least its declaration visible, or add a comment
> specifying its type.
The head declaration is in the hidden doctest setup code (marked with
`# `). Since it's setup code that would be done by C in practice,
keeping it hidden seems right. The SAFETY comment on the clist_create!
call now documents what head is.
> > + pub unsafe fn from_raw<'a>(ptr: *mut bindings::list_head) -> &'a Self {
> > + // SAFETY:
> > + // - [`CList`] has same layout as [`CListHead`] due to
> > repr(transparent).
> > + // - Caller guarantees `ptr` is a valid, sentinel `list_head`
> > object.
> > + unsafe { &*ptr.cast() }
> > + }
>
> IIUC you can call `CListHead::from_raw` here instead of repeating its
> code.
CListHead::from_raw returns &CListHead, but we need &CList<T, OFFSET>.
Since CList is repr(transparent) over CListHead, the direct ptr.cast()
is the correct approach - we'd need an additional cast after
CListHead::from_raw anyway, which would be more code, not less.
> > + fn next(&mut self) -> Option<Self::Item> {
>
> This method is the only one not marked `#[inline]`.
Added #[inline], thanks for catching that.
> > +impl<'a> FusedIterator for CListHeadIter<'a> {}
>
> I asked this a couple of times ([1], [2]) but got no reply, so let me
> try again. :) Given that `list_head` is doubly-linked, can we also
> implement `DoubleEndedIterator`?
Apologies for the missed replies! Yes, DoubleEndedIterator makes sense
for doubly-linked lists. I'll add it as a follow-up patch since it
requires a prev() method and some additional iterator state tracking.
> > +// SAFETY: [`CListHead`] can be sent to any thread.
> > +unsafe impl Send for CListHead {}
>
> That safety comment is circular. I guess you mean to say that
> `list_head` can be sent to any thread?
Fixed. Updated to: "list_head contains no thread-bound state; it only
holds next/prev pointers."
> Since Rust macros let us be more creative, how about something like
> this:
>
> let list = unsafe { clist_create!(head => Item, SampleItemC.link) };
Interesting idea! For now I've kept the current comma-separated syntax
but with the device.rs unsafe pattern:
clist_create!(unsafe { head, Item, SampleItemC, link })
This keeps consistency with other kernel macros. The unsafe { } block
is the key improvement from Danilo's review. We can explore syntax
sugar in a follow-up if desired.
Thanks,
Joel
