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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7936 bytes --]

Hi Paolo

On Mon, Sep 13, 2021 at 9:18 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> 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


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 11010 bytes --]