Hi On Fri, Sep 11, 2020 at 7:17 PM Paolo Bonzini <pbonz...@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