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

[Martin Rodriguez Reboredo]

On 6/2/23 07:18, Qingsong Chen wrote:
> SgTable is similar to `struct sg_table`, consisted of
> scatterlist entries, and could be chained with each other.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@xxxxxxxxxxxx>
> ---
> [...]
> +
> +impl<'a, const N: usize> SgTable<'a, N> {
> [...]
> +
> +    /// Chain two [`SgTable`] together.
> +    ///
> +    /// Transfer the last entry of this table as a chainable pointer to the
> +    /// first entry of `sgt` SgTable.
> +    pub fn chain_sgt<const M: usize>(&mut self, sgt: Pin<&mut SgTable<'_, M>>) -> Result {
> +        if self.count() != N - 1 {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].unmark_end();
> +
> +        let next = ScatterList::as_mut(sgt.entries[0].opaque.get()).ok_or(EINVAL)?;
> +        self.entries[N - 1].chain_sgl(next);
> +        Ok(())
> +    }
> +
> +    /// Chain [`SgTable`] and [`ScatterList`] together.
> +    ///
> +    /// Transfer the last entry of this table as a chainable pointer to `sgl`
> +    /// scatterlist.
> +    pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) -> Result {
> +        if self.count() != N - 1 {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].unmark_end();
> +        self.entries[N - 1].chain_sgl(sgl);
> +        Ok(())
> +    }
> +
> +    /// Split the first table from chained scatterlist.
> +    pub fn split(&mut self) -> Result {
> +        if !self.entries[N - 1].is_chain() {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].mark_end();
> +        Ok(())
> +    }

To these methods I'd suggest adding an `# Errors` section in the doc
comment to explain why would you get an `EINVAL`.

> +
> [...]
> +
> +    /// Get an iterator for immutable entries.
> +    pub fn iter(&self) -> Iter<'_> {
> +        Iter(ScatterList::as_ref(self.entries[0].opaque.get()))
> +    }
> +
> +    /// Get an iterator for mutable entries.
> +    pub fn iter_mut(&mut self) -> IterMut<'_> {
> +        IterMut(ScatterList::as_mut(self.entries[0].opaque.get()))
> +    }

Same as the previous commit, the `SAFETY` comment.

> +}