Re: [RFC v3 12/32] rust: provide a common crate for QEMU

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/09/21 14:19, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau<marcandre.lureau@redhat.com>
> 
> This crates provides common bindings and facilities for QEMU C API
> shared by various projects.
> 
> Most importantly, it defines the conversion traits used to convert from
> C to Rust types. Those traits are largely adapted from glib-rs, since
> those have proved to be very flexible, and should guide us to bind
> further QEMU types such as QOM. If glib-rs becomes a dependency, we
> should consider adopting glib translate traits. For QAPI, we need a
> smaller subset.
> 
> Cargo.lock is checked-in, as QEMU produces end-of-chain binaries, and it
> is desirable to track the exact set of packages that are involved in
> managed builds.
> 
> Signed-off-by: Marc-André Lureau<marcandre.lureau@redhat.com>

As in my previous review, the main issue I have here is with the 
complexity of this code.

I understand that this has to be manually written, but right now I find 
it really hard to understand what is going on here.  The patch needs to 
be expanded in several parts:

1) generic traits (including implementations for Option/Box)

2) implementation of the generic traits

3) io/nix errors

and these parts should be moved around to the place where they become 
necessary.


Also regarding the code itself:

1) Stash should not be a tuple.  Accesses to it should use standard Rust 
methods, such as borrow()/borrow_mut(), and it should also support 
standard Rust idioms such as map():

pub struct BorrowedMutPointer<'a, P, T: 'a> {
     native: *mut P,
     storage: T,
     _marker: PhantomData<&'a P>,
}

#[allow(dead_code)]
impl<'a, P: Copy, T: 'a> BorrowedMutPointer<'a, P, T> {
     fn as_ptr(&self) -> *const P {
         self.native
     }

     fn as_mut_ptr(&mut self) -> *mut P {
         self.native
     }

     fn map<U: 'a, F: FnOnce(T) -> U>(self, f: F) -> 
BorrowedMutPointer<'a, P, U> {
         BorrowedMutPointer {
             native: self.native,
             storage: f(self.storage),
             _marker: PhantomData,
         }
     }
}

impl<'a, P, T> Borrow<T> for BorrowedMutPointer<'a, P, T> {
     fn borrow(&self) -> &T {
         &self.storage
     }
}

impl<'a, P, T> BorrowMut<T> for BorrowedMutPointer<'a, P, T> {
     fn borrow_mut(&mut self) -> &mut T {
         &mut self.storage
     }
}

2) Does ToQemuPtr need to allow multiple implementations?  Can the type 
parameter in ToQemuPtr<'a, P> be an associated type (for example 
"Native")?  Type parameters really add a lot of complexity.

3) I would rather not have "qemu" in the names.  The Rust parts *are* 
QEMU.  So "foreign" or "c" would be better.

4) full/none is still really confusing to me.  I have finally understood 
that it's because the pair that borrows is from_qemu_full/to_qemu_none, 
and the pair that copies is from_qemu_none/to_qemu_full.  I'd really 
like to use names following the Rust naming conventions.  A possible 
improvement of my proposal from the previous review:

- from_qemu_full -> from_foreign (or from_c, same below)
                     + possibly a dual method into_native or into_rust

- from_qemu_none -> cloned_from_foreign

- to_qemu_none -> as_foreign or as_foreign_mut

- to_qemu_full -> clone_to_foreign

I will see if I have some time to do some of this work.

Paolo