Re: [PATCH v2 08/11] rust/block: Add driver module

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8763 bytes --]

On Tue, Feb 18, 2025 at 07:20:16PM +0100, Kevin Wolf wrote:
> This adds a barebones module for a block driver interface. Because there
> is no native QAPI support for Rust yet, opening images takes a few
> unsafe functions to call into C visitor functions. This should be
> cleaned up later.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  rust/block/src/driver.rs | 195 +++++++++++++++++++++++++++++++++++++++
>  rust/block/src/lib.rs    |   1 +
>  2 files changed, 196 insertions(+)
>  create mode 100644 rust/block/src/driver.rs
> 
> diff --git a/rust/block/src/driver.rs b/rust/block/src/driver.rs
> new file mode 100644
> index 0000000000..fe19f4b88f
> --- /dev/null
> +++ b/rust/block/src/driver.rs
> @@ -0,0 +1,195 @@
> +// Copyright Red Hat Inc.
> +// Author(s): Kevin Wolf <kwolf@redhat.com>
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +// All of this is unused until the first block driver is added
> +#![allow(dead_code)]
> +#![allow(unused_macros)]
> +#![allow(unused_imports)]
> +
> +use crate::{IoBuffer, SizedIoBuffer};
> +use qemu_api::bindings;
> +use std::ffi::c_void;
> +use std::io::{self, Error, ErrorKind};
> +use std::mem::MaybeUninit;
> +use std::ptr;
> +
> +/// A trait for writing block drivers.
> +///
> +/// Types that implement this trait can be registered as QEMU block drivers using the
> +/// [`block_driver`] macro.
> +pub trait BlockDriver {

> +    /// The type that contains the block driver specific options for opening an image
> +    type Options;
> +
> +    // TODO Native support for QAPI types and deserialization
> +    unsafe fn parse_options(
> +        v: &mut bindings::Visitor,
> +        opts: &mut *mut Self::Options,
> +        errp: *mut *mut bindings::Error,
> +    );
> +    unsafe fn free_options(opts: *mut Self::Options);
> +    unsafe fn open(
> +        bs: *mut bindings::BlockDriverState,
> +        opts: &Self::Options,
> +        errp: *mut *mut bindings::Error,
> +    ) -> std::os::raw::c_int;
> +
> +    /// Returns the size of the image in bytes
> +    fn size(&self) -> u64;

I wonder if it makes sense to separate different parts of the
BlockDriver API into multiple traits:
1. Pre-open APIs (open(), probe()?, etc)
2. I/O path - Future or async-based and can run in any thread
3. Control path (size(), close(), reopen()?, etc)

That way the type system guarantees that sub-APIs that are only valid in
a specific state (e.g. when the BlockDriverState is open) cannot be
called.

size() is an example of a method that would benefit from more
fine-grained traits that keep states separated. size() -> u64 doesn't
make sense before open().

What do you think?

> +}
> +
> +/// Represents the connection between a parent and its child node.
> +///
> +/// This is a wrapper around the `BdrvChild` type in C.
> +pub struct BdrvChild {
> +    child: *mut bindings::BdrvChild,
> +}
> +
> +// TODO Represent the graph lock and let the compiler verify it's held when accessing child
> +unsafe impl Send for BdrvChild {}
> +unsafe impl Sync for BdrvChild {}
> +
> +impl BdrvChild {
> +    /// Creates a new child reference from a `BlockdevRef`.
> +    pub unsafe fn new(
> +        parent: *mut bindings::BlockDriverState,
> +        bref: *mut bindings::BlockdevRef,
> +        errp: *mut *mut bindings::Error,
> +    ) -> Option<Self> {
> +        unsafe {
> +            let child_bs = bindings::bdrv_open_blockdev_ref_file(bref, parent, errp);
> +            if child_bs.is_null() {
> +                return None;
> +            }
> +
> +            bindings::bdrv_graph_wrlock();
> +            let child = bindings::bdrv_attach_child(
> +                parent,
> +                child_bs,
> +                c"file".as_ptr(),
> +                &bindings::child_of_bds as *const _,
> +                bindings::BDRV_CHILD_IMAGE,
> +                errp,
> +            );
> +            bindings::bdrv_graph_wrunlock();
> +
> +            if child.is_null() {
> +                None
> +            } else {
> +                Some(BdrvChild { child })
> +            }
> +        }
> +    }
> +
> +    /// Reads data from the child node into a linear byte buffer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `buf` must be a valid I/O buffer that can store at least `bytes` bytes.
> +    pub async unsafe fn read_raw(&self, offset: u64, bytes: usize, buf: *mut u8) -> io::Result<()> {
> +        let offset: i64 = offset
> +            .try_into()
> +            .map_err(|e| Error::new(ErrorKind::InvalidInput, e))?;
> +        let bytes: i64 = bytes
> +            .try_into()
> +            .map_err(|e| Error::new(ErrorKind::InvalidInput, e))?;
> +
> +        let ret = unsafe { bindings::bdrv_pread(self.child, offset, bytes, buf as *mut c_void, 0) };
> +        if ret < 0 {
> +            Err(Error::from_raw_os_error(-ret))
> +        } else {
> +            Ok(())
> +        }
> +    }
> +
> +    /// Reads data from the child node into a linear typed buffer.
> +    pub async fn read<T: IoBuffer + ?Sized>(&self, offset: u64, buf: &mut T) -> io::Result<()> {
> +        unsafe {
> +            self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
> +                .await
> +        }
> +    }
> +
> +    /// Reads data from the child node into a linear, potentially uninitialised typed buffer.
> +    pub async fn read_uninit<T: SizedIoBuffer>(
> +        &self,
> +        offset: u64,
> +        mut buf: Box<MaybeUninit<T>>,
> +    ) -> io::Result<Box<T>> {
> +        unsafe {
> +            self.read_raw(offset, buf.buffer_len(), buf.buffer_mut_ptr())
> +                .await?;
> +            let ptr = Box::into_raw(buf) as *mut T;
> +            Ok(Box::from_raw(ptr))
> +        }
> +    }
> +}
> +
> +#[doc(hidden)]
> +pub unsafe extern "C" fn bdrv_open<D: BlockDriver>(
> +    bs: *mut bindings::BlockDriverState,
> +    options: *mut bindings::QDict,
> +    _flags: std::os::raw::c_int,
> +    errp: *mut *mut bindings::Error,
> +) -> std::os::raw::c_int {
> +    unsafe {
> +        let v = match bindings::qobject_input_visitor_new_flat_confused(options, errp).as_mut() {
> +            None => return -libc::EINVAL,
> +            Some(v) => v,
> +        };
> +
> +        let mut opts: *mut D::Options = ptr::null_mut();
> +        D::parse_options(v, &mut opts, errp);
> +        bindings::visit_free(v);
> +
> +        let opts = match opts.as_mut() {
> +            None => return -libc::EINVAL,
> +            Some(opts) => opts,
> +        };
> +
> +        while let Some(e) = bindings::qdict_first(options).as_ref() {
> +            bindings::qdict_del(options, e.key);
> +        }
> +
> +        let ret = D::open(bs, opts, errp);
> +        D::free_options(opts);
> +        ret
> +    }
> +}
> +
> +#[doc(hidden)]
> +pub unsafe extern "C" fn bdrv_close<D: BlockDriver>(bs: *mut bindings::BlockDriverState) {
> +    unsafe {
> +        let state = (*bs).opaque as *mut D;
> +        ptr::drop_in_place(state);
> +    }
> +}
> +
> +/// Declare a format block driver. This macro is meant to be used at the top level.
> +///
> +/// `typ` is a type implementing the [`BlockDriver`] trait to handle the image format with the
> +/// user-visible name `fmtname`.
> +macro_rules! block_driver {
> +    ($fmtname:expr, $typ:ty) => {
> +        const _: () = {
> +            static mut BLOCK_DRIVER: ::qemu_api::bindings::BlockDriver =
> +                ::qemu_api::bindings::BlockDriver {
> +                    format_name: ::qemu_api::c_str!($fmtname).as_ptr(),
> +                    instance_size: ::std::mem::size_of::<$typ>() as i32,
> +                    bdrv_open: Some($crate::driver::bdrv_open::<$typ>),
> +                    bdrv_close: Some($crate::driver::bdrv_close::<$typ>),
> +                    bdrv_child_perm: Some(::qemu_api::bindings::bdrv_default_perms),
> +                    is_format: true,
> +                    ..::qemu_api::zeroable::Zeroable::ZERO
> +                };
> +
> +            qemu_api::module_init! {
> +                MODULE_INIT_BLOCK => unsafe {
> +                    ::qemu_api::bindings::bdrv_register(std::ptr::addr_of_mut!(BLOCK_DRIVER));
> +                }
> +            }
> +        };
> +    };
> +}
> +pub(crate) use block_driver;
> diff --git a/rust/block/src/lib.rs b/rust/block/src/lib.rs
> index 1c03549821..54ebd480ec 100644
> --- a/rust/block/src/lib.rs
> +++ b/rust/block/src/lib.rs
> @@ -1,3 +1,4 @@
> +mod driver;
>  mod iobuffer;
>  
>  pub use iobuffer::{IoBuffer, SizedIoBuffer};
> -- 
> 2.48.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]