Re: [PATCH RFC 1/4] rust: clist: Add abstraction for iterating over C linked lists

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Danilo,

On 10/30/2025 5:15 PM, Danilo Krummrich wrote:
> On Thu Oct 30, 2025 at 8:06 PM CET, Joel Fernandes wrote:
>> Provides a safe interface for iterating over C's intrusive
> 
> I'm not sure we're there just yet, I count eight unsafe blocks in the subsequent
> sample code.
> 
> Don't get me wrong, there is no way to make the API safe entirely, but "safe
> interface" is clearly a wrong promise. :)

Well, to be fair most of the unsafe statements are related to bindings, not
clist per-se. There are 8 unsafe references in the sample, of these 3 are
related to clist.  The remaining 5 are bindings/ffi related. However, I am Ok
with removing the mention of 'safe' from this comment.

> 
> Some more high level comments below.
> 
>> +//! // Rust-side struct to hold pointer to C-side struct.
>> +//! struct Item {
>> +//!     ptr: NonNull<bindings::c_item>,
>> +//! }
>> +//!
>> +//! impl clist::FromListHead for Item {
>> +//!     unsafe fn from_list_head(link: *const bindings::list_head) -> Self {
>> +//!         let item_ptr = container_of!(link, bindings::c_item, link) as *mut _;
>> +//!         Item { ptr: NonNull::new_unchecked(item_ptr) }
>> +//!     }
>> +//! }
> 
> If you just embedd a pointer to the C struct in a struct Item you don't cover
> the lifetime relationship.
> 
> Instead this should be something like
> 
> 	#[repr(transparent)]
> 	struct Entry<T>(Opaque<T>);
> 
> or
> 
> 	struct Entry<'a, T>(NonNull<T>, PhantomData<&'a T>);
> 
> where T is the C list entry type.
> 
> You can then have a setup where an &Entry borrows from a &CListHead, which
> borrows from a Clist.

Yes, in my implementation my iterator creates a new value on in iterator's
next() function without lifetime relationships:

            // Use the trait to convert list_head to T.
            Some(T::from_list_head(self.current))

I think you're saying that we should have a lifetime relationship between the
rust Clist and the rust entry (item) returned by from_list_head() right?

Your suggestion makes sense for cases where a rust Item/Entry has a Drop
implementation that then makes the C side free the object and its list_head. DRM
Buddy does not have this requirement, however your suggestion makes the code
future proof and better. Basically, the rust item wrapper must outlive the clist
view is what you're saying, I think. That can be achieved by making the rust
item borrow from the clist. I will work on this accordingly then.

> I'd also provide a macro for users to generate this structure as well as the
> corresponding FromListHead impl.
> 
>> +//! // Rust wraps and iterates over the list.
>> +//! let list = unsafe { clist::Clist::<Item>::new(c_list_head) };
> 
> This function has a lot of safety requirements that need to be covered.

Sure, I can add those to the example as well.

> 
> It also should be, besides the unsafe FromListHead trait, the only unsafe
> function needed.
> 
> The Clist should ideally have methods for all kinds of list iterators, e.g.
> list_for_each_entry_{safe,reverse,continue}() etc.
> 
> Of course you don't need to provide all of them in an initial implementation,
> but we should set the direction.

Yes, initially I am trying to only provide most common thinks, especially what
the DRM Buddy usecase needs. Happy to improve it over time.

thanks,

 - Joel