Re: [PATCH] PoC: Rust binding for QAPI (qemu-ga only, for now)

[Marc-André Lureau]

Hi

On Fri, Sep 11, 2020 at 7:17 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

On 11/09/20 16:00, Marc-André Lureau wrote:
>     - from_qemu_none should be a "to_*" or "constructor" conversion (I used
>     new_from_foreign)
>
> new_ prefix is not very rusty.

Right, I have changed it to with_foreign so now there is
{as,with,to,into}_foreign, plus unsafe_{from,into}.

These two could even be renamed to from_foreign and into_native at the
cost of making the trait less general purpose.  This way we have the
typical Rust names: as_* for Borrowed->Borrowed, with_*/to_* for
Borrowed->Owned, from_*/into_* for Owned->Owned.

> However, the memory allocator (or the stack) may not be compatible
> with the one used in C.

Hmm, that's a good point.  The simplest solution might be just to get
rid of IntoForeign, it's just an optimization.

> from_raw() is common, and takes ownership.

from_raw()/into_raw() would be equivalent to
into_foreign()/from_foreign().  However as you point out the allocators
are different, so it's a good idea IMHO to separate
into_raw()/from_raw() for the Rust allocator from
into_foreign()/from_foreign() for the libc allocator.

> I would need to modify this PoC for example

Yes of course.  Can you just try splitting the PoC in multiple patches?
 That should also make it easier to review, so far all I did was
comparing against glib-rs.

> But I must say I feel quite comfortable with the glib approach. It
> would be nice to have some feedback from glib-rs maintainers about your
> proposal.

QAPI is not tied to glib-rs, so I don't think qemu-ga will need to use
glib-rs.  I think either we use glib-rs, or if we are to roll our own we
should not be tied to the naming.  We don't use GObject introspection,
so none/full means nothing to most QEMU developers (and to Rust
developers too).

There are other things I don't like very much in glib-rs, for example
the use of tuples and public fields and the somewhat messy usage of
*const/*mut (I tried to be stricter on that).

I am trying to wrap my head around your proposal (based on https://github.com/bonzini/rust-ptr), and trying to understand the limitations/unrustiness of the glib-rs translate traits I used in this PoC.

First let's clarify the requirements. We need those conversions for now:
- const *P -> T
- mut *P -> T
And:
- &T -> const *P
- &T -> mut *P

Note that glib-rs has more advanced conversions, because of partial ownership transfer with containers, and ref-counted types etc. Those could soon become necessary for QEMU to bind other types than QAPI, in particular QOM and our usage of glib in general. I kept that in mind by carefully choosing glib-rs as a reference. I think it's important to take it into account from the start (sadly, some limitations don't allow us to simply use glib-rs traits, for reasons that aren't 100% clear to me, but are clear to the compiler and others :)

Some other remarks:
- "mut *P -> T" is often just "const *P -> T" with P being freed after conversion
- "&T -> const *P" can be "&T -> mut *P" with Rust side freeing P after usage thanks to a stash, but can also be very different and not require it (strings for example, the constP uses CString, while the mutP version is just a g_strndup)
- it is nice (or necessary) to have to allow some form of composition for container-like types (Option<T>, Vec<T>, struct T(U,V) inside etc) to avoid duplication
- Rust naming conventions guide us towards using to_ and into_ (for owned->owned) prefixes.


The glib-rs traits map the conversion functions respectively to (I removed the Glib/Qemu prefix, because the subset used in both are very close):
- FromPtrNone::from_none
- FromPtrFull::from_full (usually just calls from_none() and free(P))
And:
- ToPtr::to_none (with the Stash)
- ToPtr::to_full

The symmetry is clear, and arguably easy to remember. fwiw, I don't know why ToPtr wasn't split the same way FromPtr was (they used to be on the same FromPtr trait).

The usage of to_ prefix is in accordance with the Rust conventions here. The usage of from_ is perhaps not ideal?, but from_full is not incompatible with the symmetrical into_ (as in From<T> for U implies Into<U> for T).

Experience shows that the combination of Stash & ToPtr design makes it convenient for type composition too.


My understanding of what you propose is:
- ForeignConvert::with_foreign
- FromForeign::from_foreign (with implied into_native)
And:
- ForeignConvert::as_foreign (with the BorrowedPointer/stash-like)
- ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems wrongly designed in your proposal and unnecessary for now)

I excluded IntoForeign::into_foreign, since "T -> P" can't really be done better than "&T -> *P" due to different memory allocators etc.

I don't have your head, so I find it hard to remember & work with. It uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_. That just blows my mind, sorry :)

Then, I don't understand why ForeignConvert should hold both the "const *P -> T" and "&T -> const *P" conversions. Except the common types, what's the relation between the two?

Finally, I thought you introduced some differences with the stash design, but in fact I can see that ForeignConvert::Storage works just the way as ToPtr::Storage. So composition should be similar. Only your example code is more repetitive as it doesn't indirectly refer to the trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage).

I am not making any conclusions yet, but I am not exactly happily going to switch to your proposal yet :)

Comments?




 

--

Marc-André Lureau