Re: [PATCH 06/11] rust/block: Add I/O buffer traits

[Kevin Wolf <kwolf@redhat.com>]

Am 12.02.2025 um 17:48 hat Paolo Bonzini geschrieben:
> On 2/11/25 22:43, Kevin Wolf wrote:
> > +/// Implementing `SizedIoBuffer` provides an implementation for [`IoBuffer`] without having to
> > +/// implement any functions manually.
> > +///
> > +/// # Safety
> > +///
> > +/// Types implementing `SizedIoBuffer` guarantee that the whole object can be accessed as an I/O
> > +/// buffer that is safe to contain any byte patterns.
> > +pub unsafe trait SizedIoBuffer: Sized {
> 
> This is similar to the ByteValued trait in rust-vmm.  Can you name it
> the same so that we can later consider replacing it?

I'm not sure if it's the best name, but could be done, of course.

Though the more interesting thing to replace it with eventually might be
the zerocopy crate which has derive macros that check that the condition
is actually fulfilled. I just didn't feel like bringing in new external
dependencies in this first series.

> > +    fn from_byte_slice(buf: &[u8]) -> Option<&Self> {
> > +        if buf.len() < std::mem::size_of::<Self>() {
> > +            return None;
> > +        }

This is a semantic difference compared to ByteValued::from_slice(),
which requires the sizes to match exactly. For the probe function, I
actually make use of the relaxed requirement here to access a header
struct in a larger buffer passed from C.

> > +        let ptr = buf.as_ptr() as *const Self;
> > +
> > +        // TODO Use ptr.is_aligned() when MSRV is updated to at least 1.79.0
> > +        if (ptr as usize) % std::mem::align_of::<Self>() != 0 {
> > +            return None;
> > +        }
> > +
> > +        // SAFETY: This function checked that the byte slice is large enough and aligned.
> > +        // Implementing SizedIoBuffer promises that any byte pattern is valid for the type.
> > +        Some(unsafe { &*ptr })
> 
> If you want, the function can be written also
> 
>     // SAFETY: implementing SizedIoBuffer promises that any byte pattern
>     // is valid for the type
>     match unsafe { buf.align_to::<Self>() } {
>         ([], mid, _) => mid.get(0),
>         _ => None
>     }
> 
> (trick stolen from rust-vmm, in fact).

Clever way to avoid ptr::is_aligned(), but I feel a bit harder to
understand than just open-coding it like above? (And probably less
efficient, but I don't know how relevant that is.)

Kevin