On Fri, Sep 27, 2024 at 6:10 PM Kevin Wolf <kw...@redhat.com> wrote:
> Am 01.07.2024 um 16:58 hat Paolo Bonzini geschrieben:
> > The qemu::util::foreign module provides:
> >
> > - A trait for structs that can be converted to a C ("foreign")
> > representation
> > (CloneToForeign)
> >
> > - A trait for structs that can be built from a C ("foreign") representation
> > (FromForeign), and the utility IntoNative that can be used with less
> > typing
> > (similar to the standard library's From and Into pair)
>
> It makes sense to me that we'll need something to convert data and that
> this usually means creating a new instance, i.e. cloning. However, while
> it's obvious that this is similar to From/Into, the part I'm missing
> here is what's different from it.
>
> In other words, couldn't we just implement the normal From trait between
> FFI types and the native equivalent?
In general yes. Using new traits has two advantages (all IMHO of
course). First, it makes it possible to tie the implementation to the
availability of a freeing function; second, it makes it possible to
define this always, whereas From is limited by the orphan rule (you
cannot provide implementations of a trait for a struct unless you are
the crate that defines either the struct or the trait).
> > - Automatic implementations of the above traits for Option<>, supporting
> > NULL
> > pointers
>
> This is nice.
... for example, you can't have such a blanket implementation "impl<T,
U: From<T>> From<T> for Option<U> {}".
> > - A wrapper for a pointer that automatically frees the contained data. If
> > a struct XYZ implements CloneToForeign, you can build an OwnedPointer<XYZ>
> > and it will free the contents automatically unless you retrieve it with
> > owned_ptr.into_inner()
>
> Something here feels off to me.
>
> At first, I thought it might be only about naming. This is not about
> owning the pointer (which you probably do anyway), but that the pointer
> owns the object it points to. This concept has in fact a name in Rust:
> It's a Box.
>
> The major difference compared to Box is that we're using a different
> allocator. Not sure if the allocator APIs would actually be viable, but
> they're not stable anyway - but let's at least name this thing in way
> that draws the obvious parallel. Maybe ForeignBox.
> But the other thing that doesn't feel quite right is how this is coupled
> with CloneToForeign. Freeing is different from cloning, and it really
> relates to the foreign type itself, and not to the one that can be
> cloned into a foreign type.
I am not 100% convinced of the ForeignBox name but I see your point.
You'd prefer to have "impl FreeForeign for c_char" and "impl
CloneToForeign for str", where it's the CloneToForeign::Foreign
associated type (in this case c_char) that must implement FreeForeign.
Also, clone_to_foreign() for a str would return an
OwnedPointer<c_char>, not an OwnedPointer<str>. Never be too
optimistic about Rust, but that should be doable and I agree it is
better.
> Bringing both together, what a Box doesn't usually have is a function
> pointer for freeing. We probably don't need it here either, almost
> everything needs g_free().
Is that true? For example we could have qobject_unref, or qapi_free_*
as implementations of FreeForeign.
> There is a different free_foreign()
> implementation for Error, but arguably this could be changed:
> bindings::Error should then implement Drop for the inner value (with an
> error_free_inner() which needs to be exported separately from C first),
> and then ForeignBox can just drop the Error object and g_free() the
> pointer itself like it would do with any other value.
I think that's a bit too magical. Making Error always represent an
owned object seems a bit risky; instead I'm treating the bindings::XYZ
structs as very C-like objects, that are usually managed through
either OwnedPointer<...> (e.g. for a parameter) or *mut XYZ (for a
return value).
Thanks for looking at this. This kind of review is something that
we'll have to do a lot and it's better to have some early experience
of what they look like, independent of whether this code ultimately
will or will not be part of QEMU.
Paolo
