Re: [RFC PATCH 1/2] rust: add initial scatterlist bindings

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

Hi Lyude,

On 15/05/2025 23:01, Lyude Paul wrote:
> 
> I think you might have made a mistake below
> 
>> +impl<'a> Iterator for SGTableIter<'a> {
>> +    type Item = &'a SGEntry;
>> +
>> +    fn next(&mut self) -> Option<Self::Item> {
>> +        self.pos.map(|entry| {
>> +            let sg = entry.as_raw();
> 
>                 ^ sg is the last iterator position
> 
>> +            // SAFETY: `sg` is guaranteed to be valid and non-NULL while inside this closure.
>> +            unsafe {
>> +                let next = bindings::sg_next(sg);
> 
>                     ^ and we fetch the next iterator position
> 
>> +                self.pos = (!next.is_null()).then(|| SGEntry::as_ref(next));
>> +                SGEntry::as_ref(sg)
> 
>                     ^ but then we return the previous iterator position, sg
>> +            }
> 

Just had a second look at this again.I think the previous code is okay. 
I integrated the iterator fixes you suggested, however the mapped 
sgtable crashes on dma_unmap_sgtable().

One hint is that the fix seems to skip the initial entry of the sgtable? 
The previous code doesn't skip the initial entry when iterating:

fn next(&mut self) -> Option<Self::Item> {

   self.pos.take().map(|entry| {
      let sg = entry.as_raw();
      ^ sg is the current iterator position from self.pos

      let next = bindings::sg_next(sg);
      self.pos = (!next.is_null()).then(|| SGEntry::as_mut(next));
      ^ update the iterator position for the next loop to sg_next()
        or set to null if sg_next() is the last entry

      SGEntry::as_mut(sg)
      ^ Return the current iterator position
    })
}


Regards,
Abdiel