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