On 07/09/21 14:19, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau<marcandre.lur...@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.lur...@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
