Re: [PATCH v2 1/2] rust: add scatterlist support

[Martin Rodriguez Reboredo <yakoyoku@gmail.com>]

On 4/5/23 17:14, Daniel Almeida wrote:
> This patch adds a scatterlist abstraction. It is then used and tested
> by the new rust virtio sample module.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/lib.rs         |  1 +
>  rust/kernel/scatterlist.rs | 40 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>  create mode 100644 rust/kernel/scatterlist.rs
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index c20b37e88ab2..b23be69919cc 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -71,6 +71,7 @@ pub mod net;
>  pub mod pages;
>  pub mod power;
>  pub mod revocable;
> +pub mod scatterlist;
>  pub mod security;
>  pub mod task;
>  pub mod workqueue;
> diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
> new file mode 100644
> index 000000000000..fe036af13995
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Scatterlist abstractions.
> +//!
> +//! C header: [`include/linux/virtio.h`](../../../../include/linux/scatterlist.h)

Little typo, I think that you've meant this to be scatterlist.h instead
of virtio.h.

> +
> +use core::cell::UnsafeCell;
> +
> +/// Scatterlist abstraction over the C side `struct scatterlist`.
> +pub struct Scatterlist {
> +    /// The C side `struct scatterlist`.
> +    inner: UnsafeCell<bindings::scatterlist>,
> +}
> +

Wouldn't be better to use a transparent tuple struct [1] since this only
has one non-zero sized field and could be easily transmuted into its
inner type.

> +impl Scatterlist {
> +    /// A wrapper over the C-side `sg_init_one()`. Initialize a single entry sg
> +    /// list.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Caller must ensure that `buf` is valid and `buflen` really
> +    /// represents the size of `buf`.
> +    pub unsafe fn init_one(buf: *const core::ffi::c_void, buflen: u32) -> Self {
> +        // SAFETY: There are no references or function pointers in this
> +        // `Scatterlist`.
> +        let mut sg: Scatterlist = unsafe { core::mem::zeroed() };
> +        // SAFETY: we pass a valid sg entry, which points to stack-allocated
> +        // variable above. `buf` and `buflen` are presumed valid as per this
> +        // function's safety requirements.
> +        unsafe {
> +            bindings::sg_init_one(sg.inner.get_mut(), buf, buflen);
> +        }
> +
> +        sg
> +    }

If this function was meant to be used for internal use only then it
should be `pub(crate)`, or else, a function that takes an slice should
be provided as a safe alternative.

> +
> +    pub(crate) fn inner(&self) -> &UnsafeCell<bindings::scatterlist> {
> +        &self.inner
> +    }
> +}

Link: https://doc.rust-lang.org/nomicon/other-reprs.html#reprtransparent [1]
Regards
-> Martin