Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction

[Qingsong Chen]

On 6/3/23 3:54 AM, Martin Rodriguez Reboredo wrote:
> On 6/2/23 07:18, Qingsong Chen wrote:
> > +/// # Invirants
> > +///
> > +/// All instances should be valid, either created by the `new` constructor
> > +/// (see [`pin_init`]), or transmuted from raw pointers (which could be used
> > +/// to reference a valid entry of `sg_table`).
> > +///
> > +/// # Examples
> > +///
> > +/// The following is some use cases of [`ScatterList`].
> > +///
> > +/// ```rust
> > +/// use core::pin::pin;
> > +/// # use kernel::error::Result;
> > +/// # use kernel::scatterlist::ScatterList;
> > +///
> > +/// // Prepare memory buffers.
> > +/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
> > +/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
> > +///
> > +/// // Allocates an instance on stack.
> > +/// kernel::stack_pin_init!(let foo = ScatterList::new(&buf0));
> > +/// let mut foo: Pin<&mut ScatterList<'_>> = foo;
> > +/// assert_eq!(foo.length(), 512);
> > +///
> > +/// // Alloccate an instance by Box::pin_init.
> > +/// let bar: Pin<Box<ScatterList<'_>>> = Box::pin_init(ScatterList::new(&buf1))?;
> > +/// assert_eq!(bar.length(), 512);
>
> This comment has some typos, but luckily there's the `typos` tool [1]
> out there to help us.

I just tried the `typos` tool, but it didn't report any errors. Though, 
I would try other spell checker, with more caution. Thanks.

> > +    /// Obtain a pinned reference to an immutable scatterlist from a raw pointer.
> > +    pub fn as_ref(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a Self>> {
> > +        // SAFETY: `sgl` is non-null and valid.
> > +        NonNull::new(ptr).map(|sgl| Pin::new(unsafe { &*(sgl.as_ptr() as *const ScatterList<'_>) }))
> > +    }
> > +
> > +    /// Obtain a pinned reference to a mutable scatterlist from a raw pointer.
> > +    pub fn as_mut(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a mut Self>> {
> > +        // SAFETY: `sgl` is non-null and valid.
> > +        NonNull::new(ptr)
> > +            .map(|sgl| Pin::new(unsafe { &mut *(sgl.as_ptr() as *mut ScatterList<'_>) }))
> > +    }
>
> Please mark both of these as `unsafe` as you can still pass an invalid
> pointer to them. Also, if there's no other user of those methods other
> than this module then I'd suggest to make them private.

Those methods are only used by `SgTable` and the iterators by design. 
And it would be better if they were private and marked as unsafe. Thanks.