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

["Qingsong Chen" <changxian.cqs@antgroup.com>]

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.