[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC v3 12/32] rust: provide a common crate for QEMU
From: |
Paolo Bonzini |
Subject: |
Re: [RFC v3 12/32] rust: provide a common crate for QEMU |
Date: |
Mon, 13 Sep 2021 19:11:18 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
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
- [RFC v3 08/32] tests: build qapi-cabi (C ABI dump), (continued)
- [RFC v3 08/32] tests: build qapi-cabi (C ABI dump), marcandre . lureau, 2021/09/07
- [RFC v3 09/32] build-sys: add i686 cpu target, marcandre . lureau, 2021/09/07
- [RFC v3 10/32] build-sys: add --with-rust{-target} & basic build infrastructure, marcandre . lureau, 2021/09/07
- [RFC v3 11/32] build-sys: add a cargo-wrapper script, marcandre . lureau, 2021/09/07
- [RFC v3 12/32] rust: provide a common crate for QEMU, marcandre . lureau, 2021/09/07
- [RFC v3 13/32] rust: use vendored-sources, marcandre . lureau, 2021/09/07
- Re: [RFC v3 13/32] rust: use vendored-sources, Ian Jackson, 2021/09/08
- Re: [RFC v3 13/32] rust: use vendored-sources, Marc-André Lureau, 2021/09/08
- Re: [RFC v3 13/32] rust: use vendored-sources, Ian Jackson, 2021/09/08
- Re: [RFC v3 13/32] rust: use vendored-sources, Marc-André Lureau, 2021/09/08
- Re: [RFC v3 13/32] rust: use vendored-sources, Peter Maydell, 2021/09/08
- Re: [RFC v3 13/32] rust: use vendored-sources, Ian Jackson, 2021/09/08
- Re: [RFC v3 13/32] rust: use vendored-sources, Marc-André Lureau, 2021/09/08
- Re: [RFC v3 13/32] rust: use vendored-sources, Ian Jackson, 2021/09/08