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.
The common crate can be split in many pieces. It is easier to have it as a single commit during PoC/RFC (the series is already large enough).
Is it really valuable to introduce a new crate/library piece by piece? Instead, better documentation and tests are more valuable imho. But I will certainly split it and clean it more as we iterate.
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():
I thought we already discussed that. The stash is an implementation detail to handle FFI resource release. The only thing the user should care about the stash is the pointer, which is shortly retrieved with ".0" (cannot be more succinct).
Ideally, we would like to provide "foo.to_qemu_none()", but to keep the associated resource allocated, the shortest we can offer is "foo.to_qemu_none().0". Using "as_ptr()" instead would not only be longer to type but possibly misleading: there is nothing else you should do with the stash, no other method should exist.
I don't understand what "map()" would provide here either. The storage part of the stash is an internal detail (the FFI data), not meant to be manipulated at all (it would invalidate the stash pointer).
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.
ToQemuPtr is a trait, implemented for the various Rust types that we can translate to FFI pointers.
For example:
impl<'a> ToQemuPtr<'a, *mut c_char> for String {
type Storage = CString;
...
}
The pointer type parameter is to return the correct pointer. It is not necessary to specify it as a user, since Rust infers it from the expected type.
3) I would rather not have "qemu" in the names. The Rust parts *are*
QEMU. So "foreign" or "c" would be better.
That's kind of cosmetic.
The main reason for "qemu" in the name is to avoid potential clashes with other methods and traits.
Also "to_none" could associate in the reader's mind (wrongly) with the ubiquitous Option::None.
But yes, we could drop "qemu" here.
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:
"full" and "none" are inherited from the glib-introspection ownership transfer annotations.
We discussed and weighed pros and cons last year. I haven't yet found or agreed on better options. And you can be certain that the glib-rs folks have already pondered a lot about the naming and design. And, I don't like to reinvent what others have done (creating various limitations and incompatibilities). Following the glib traits brings a lot of experience and could help us reuse glib-rs crates more easily (because they would use the same design), but also to port and interoperate. Imagine we need to bind GHashTable<char *, QapiFoo> someday, we will be in a better position if we have translation traits that follow glib-rs already.
The Rust conventions can be inferred from CString/CStr design and methods, but they have to be adjusted, because they don't solve the same constraints (no ownership transfer, and CString/CStr intermediary representations).
They are "CStr::from_ptr(*const)" and "CString::as_ptr(&self) -> *const".
- from_qemu_full -> from_foreign (or from_c, same below)
+ possibly a dual method into_native or into_rust
There is no standard naming for such FFI pointer ownership transfer. Rust doesn't know how to release FFI resources, in general.
from_raw(*mut) is the closest, but "raw" is reserved for Rust data pointer types, not FFI (except for slices, probably because the linear layout is the same as FFI)
We can consider "from_mut_ptr(*mut)", but this doesn't convey the ownership transfer as explicitly.
- from_qemu_none -> cloned_from_foreign
You are being creative! This is like "CStr::from_ptr", except we go directly to higher Rust types.
- to_qemu_none -> as_foreign or as_foreign_mut
Similarity with "CString::as_ptr", except that it handles the temporary stash (CString is the internal storage for String), so we use the "to_qemu_none().0" described earler.
If we follow your considerations it could be "to_stash().as_ptr()", which is less user friendly, exposes more internal details, etc.
- to_qemu_full -> clone_to_foreign
There is no equivalent in Rust standard. An implementation may want to store in an existing location, or allocated in different ways etc.
Closest would be to_ptr()
In summary:
from_qemu_full() vs from_mut_ptr()
from_qemu_none() from_ptr()
to_qemu_full() to_ptr()
to_qemu_none() to_stash().as_ptr()
I argue there is more consistency and readability by following the glib-rs traits and method names (among other advantages by staying close to glib-rs design for the future).
I will see if I have some time to do some of this work.
thanks