Re: [PATCH 03/14] rust: define traits and pointer wrappers to convert from/to C representations

[Kevin Wolf]

Hi Paolo,

as you asked me at KVM Forum to have a look at this, I'm going through
it now.

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?

> - Automatic implementations of the above traits for Option<>, supporting NULL
>   pointers

This is nice.

> - 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.

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(). 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.

(Your implementation actually calls free() instead of g_free(). We
generally try to avoid that in our C code, so we should probably avoid
it in Rust, too.)

Kevin