Hi
(adding Sebastian, one of the glib-rs developers in CC)
On Mon, Jul 1, 2024 at 7:02 PM Paolo Bonzini <pbonz...@redhat.com> wrote:
> 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)
>
> - Automatic implementations of the above traits for Option<>, supporting
> NULL
> pointers
>
> - 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()
>
You worry about technical debt, and I do too. Here you introduce quite
different traits than what glib-rs offers. We already touched this subject
2y ago, my opinion didn't change much (
https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/20210907121943.3498701-13-marcandre.lur...@redhat.com/).
Also, you don't offer the equivalent of "to_glib_none" which uses a
temporary stash and is quite useful, as a majority of functions don't take
ownership.
Because much of our code is using GLib types and API style, I think we
should strive for something that is close (if not just the same) to what
glib-rs offers. It's already hard enough to handle one binding concept,
having 2 will only make the matter worse. Consider a type like
GHashTable<GUuid, QOM>, it will be very annoying to deal with if we have
different bindings traits and implementations and we will likely end up
duplicating glib-rs effort.
As for naming & consistency, glib-rs settled on something clearer imho:
from_glib_full
from_glib_none
to_glib_full
to_glib_none
vs
from_foreign
cloned_from_foreign
clone_to_foreign
/nothing/
but overall, this is still close enough that we shouldn't reinvent it.
It may be worth studying what the kernel offers, I haven't checked yet.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> qemu/src/lib.rs | 6 +
> qemu/src/util/foreign.rs | 247 +++++++++++++++++++++++++++++++++++++++
> qemu/src/util/mod.rs | 1 +
> 3 files changed, 254 insertions(+)
> create mode 100644 qemu/src/util/foreign.rs
> create mode 100644 qemu/src/util/mod.rs
>
> diff --git a/qemu/src/lib.rs b/qemu/src/lib.rs
> index fce21d7..c48edcf 100644
> --- a/qemu/src/lib.rs
> +++ b/qemu/src/lib.rs
> @@ -2,3 +2,9 @@
> #![allow(dead_code)]
>
> pub mod bindings;
> +
> +pub mod util;
> +pub use util::foreign::CloneToForeign;
> +pub use util::foreign::FromForeign;
> +pub use util::foreign::IntoNative;
> +pub use util::foreign::OwnedPointer;
> diff --git a/qemu/src/util/foreign.rs b/qemu/src/util/foreign.rs
> new file mode 100644
> index 0000000..a591925
> --- /dev/null
> +++ b/qemu/src/util/foreign.rs
> @@ -0,0 +1,247 @@
> +// TODO: change to use .cast() etc.
> +#![allow(clippy::ptr_as_ptr)]
> +
> +/// Traits to map between C structs and native Rust types.
> +/// Similar to glib-rs but a bit simpler and possibly more
> +/// idiomatic.
> +use std::borrow::Cow;
> +use std::fmt;
> +use std::fmt::Debug;
> +use std::mem;
> +use std::ptr;
> +
> +/// A type for which there is a canonical representation as a C datum.
> +pub trait CloneToForeign {
> + /// The representation of `Self` as a C datum. Typically a
> + /// `struct`, though there are exceptions for example `c_char`
> + /// for strings, since C strings are of `char *` type).
> + type Foreign;
> +
> + /// Free the C datum pointed to by `p`.
> + ///
> + /// # Safety
> + ///
> + /// `p` must be `NULL` or point to valid data.
> + unsafe fn free_foreign(p: *mut Self::Foreign);
> +
> + /// Convert a native Rust object to a foreign C struct, copying
> + /// everything pointed to by `self` (same as `to_glib_full` in
> `glib-rs`)
> + fn clone_to_foreign(&self) -> OwnedPointer<Self>;
> +
> + /// Convert a native Rust object to a foreign C pointer, copying
> + /// everything pointed to by `self`. The returned pointer must
> + /// be freed with the `free_foreign` associated function.
> + fn clone_to_foreign_ptr(&self) -> *mut Self::Foreign {
> + self.clone_to_foreign().into_inner()
> + }
> +}
> +
> +impl<T> CloneToForeign for Option<T>
> +where
> + T: CloneToForeign,
> +{
> + type Foreign = <T as CloneToForeign>::Foreign;
> +
> + unsafe fn free_foreign(x: *mut Self::Foreign) {
> + T::free_foreign(x)
> + }
> +
> + fn clone_to_foreign(&self) -> OwnedPointer<Self> {
> + // Same as the underlying implementation, but also convert `None`
> + // to a `NULL` pointer.
> + self.as_ref()
> + .map(CloneToForeign::clone_to_foreign)
> + .map(OwnedPointer::into)
> + .unwrap_or_default()
> + }
> +}
> +
> +impl<T> FromForeign for Option<T>
> +where
> + T: FromForeign,
> +{
> + unsafe fn cloned_from_foreign(p: *const Self::Foreign) -> Self {
> + // Same as the underlying implementation, but also accept a
> `NULL` pointer.
> + if p.is_null() {
> + None
> + } else {
> + Some(T::cloned_from_foreign(p))
> + }
> + }
> +}
> +
> +impl<T> CloneToForeign for Box<T>
> +where
> + T: CloneToForeign,
> +{
> + type Foreign = <T as CloneToForeign>::Foreign;
> +
> + unsafe fn free_foreign(x: *mut Self::Foreign) {
> + T::free_foreign(x)
> + }
> +
> + fn clone_to_foreign(&self) -> OwnedPointer<Self> {
> + self.as_ref().clone_to_foreign().into()
> + }
> +}
> +
> +impl<T> FromForeign for Box<T>
> +where
> + T: FromForeign,
> +{
> + unsafe fn cloned_from_foreign(p: *const Self::Foreign) -> Self {
> + Box::new(T::cloned_from_foreign(p))
> + }
> +}
> +
> +impl<'a, B> CloneToForeign for Cow<'a, B>
> + where B: 'a + ToOwned + ?Sized + CloneToForeign,
> +{
> + type Foreign = B::Foreign;
> +
> + unsafe fn free_foreign(ptr: *mut B::Foreign) {
> + B::free_foreign(ptr);
> + }
> +
> + fn clone_to_foreign(&self) -> OwnedPointer<Self> {
> + self.as_ref().clone_to_foreign().into()
> + }
> +}
> +
> +/// Convert a C datum into a native Rust object, taking ownership of
> +/// the C datum. You should not need to implement this trait
> +/// as long as Rust types implement `FromForeign`.
> +pub trait IntoNative<T> {
> + /// Convert a C datum to a native Rust object, taking ownership of
> + /// the pointer or Rust object (same as `from_glib_full` in `glib-rs`)
> + ///
> + /// # Safety
> + ///
> + /// `p` must point to valid data, or can be `NULL` if Self is an
> + /// `Option` type. It becomes invalid after the function returns.
> + unsafe fn into_native(self) -> T;
> +}
> +
> +impl<T, U> IntoNative<U> for *mut T
> +where
> + U: FromForeign<Foreign = T>,
> +{
> + unsafe fn into_native(self) -> U {
> + U::from_foreign(self)
> + }
> +}
> +
> +/// A type which can be constructed from a canonical representation as a
> +/// C datum.
> +pub trait FromForeign: CloneToForeign + Sized {
> + /// Convert a C datum to a native Rust object, copying everything
> + /// pointed to by `p` (same as `from_glib_none` in `glib-rs`)
> + ///
> + /// # Safety
> + ///
> + /// `p` must point to valid data, or can be `NULL` is `Self` is an
> + /// `Option` type.
> + unsafe fn cloned_from_foreign(p: *const Self::Foreign) -> Self;
> +
> + /// Convert a C datum to a native Rust object, taking ownership of
> + /// the pointer or Rust object (same as `from_glib_full` in `glib-rs`)
> + ///
> + /// The default implementation calls `cloned_from_foreign` and frees
> `p`.
> + ///
> + /// # Safety
> + ///
> + /// `p` must point to valid data, or can be `NULL` is `Self` is an
> + /// `Option` type. `p` becomes invalid after the function returns.
> + unsafe fn from_foreign(p: *mut Self::Foreign) -> Self {
> + let result = Self::cloned_from_foreign(p);
> + Self::free_foreign(p);
> + result
> + }
> +}
> +
> +pub struct OwnedPointer<T: CloneToForeign + ?Sized> {
> + ptr: *mut <T as CloneToForeign>::Foreign,
> +}
> +
> +impl<T: CloneToForeign + ?Sized> OwnedPointer<T> {
> + /// Return a new `OwnedPointer` that wraps the pointer `ptr`.
> + ///
> + /// # Safety
> + ///
> + /// The pointer must be valid and live until the returned
> `OwnedPointer`
> + /// is dropped.
> + pub unsafe fn new(ptr: *mut <T as CloneToForeign>::Foreign) -> Self {
> + OwnedPointer { ptr }
> + }
> +
> + /// Safely create an `OwnedPointer` from one that has the same
> + /// freeing function.
> + pub fn from<U>(x: OwnedPointer<U>) -> Self
> + where
> + U: CloneToForeign<Foreign = <T as CloneToForeign>::Foreign> +
> ?Sized,
> + {
> + unsafe {
> + // SAFETY: the pointer type and free function are the same,
> + // only the type changes
> + OwnedPointer::new(x.into_inner())
> + }
> + }
> +
> + /// Safely convert an `OwnedPointer` into one that has the same
> + /// freeing function.
> + pub fn into<U>(self) -> OwnedPointer<U>
> + where
> + U: CloneToForeign<Foreign = <T as CloneToForeign>::Foreign>,
> + {
> + OwnedPointer::from(self)
> + }
> +
> + /// Return the pointer that is stored in the `OwnedPointer`. The
> + /// pointer is valid for as long as the `OwnedPointer` itself.
> + pub fn as_ptr(&self) -> *const <T as CloneToForeign>::Foreign {
> + self.ptr
> + }
> +
> + pub fn as_mut_ptr(&self) -> *mut <T as CloneToForeign>::Foreign {
> + self.ptr
> + }
> +
> + /// Return the pointer that is stored in the `OwnedPointer`,
> + /// consuming the `OwnedPointer` but not freeing the pointer.
> + pub fn into_inner(mut self) -> *mut <T as CloneToForeign>::Foreign {
> + let result = mem::replace(&mut self.ptr, ptr::null_mut());
> + mem::forget(self);
> + result
> + }
> +}
> +
> +impl<T: FromForeign + ?Sized> OwnedPointer<T> {
> + /// Convert a C datum to a native Rust object, taking ownership of
> + /// the pointer or Rust object (same as `from_glib_full` in `glib-rs`)
> + pub fn into_native(self) -> T {
> + // SAFETY: the pointer was passed to the unsafe constructor
> OwnedPointer::new
> + unsafe { T::from_foreign(self.into_inner()) }
> + }
> +}
> +
> +impl<T: CloneToForeign + ?Sized> Debug for OwnedPointer<T> {
> + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> + let name = std::any::type_name::<T>();
> + let name = format!("OwnedPointer<{}>", name);
> + f.debug_tuple(&name).field(&self.as_ptr()).finish()
> + }
> +}
> +
> +impl<T: CloneToForeign + Default + ?Sized> Default for OwnedPointer<T> {
> + fn default() -> Self {
> + <T as Default>::default().clone_to_foreign()
> + }
> +}
> +
> +impl<T: CloneToForeign + ?Sized> Drop for OwnedPointer<T> {
> + fn drop(&mut self) {
> + let p = mem::replace(&mut self.ptr, ptr::null_mut());
> + // SAFETY: the pointer was passed to the unsafe constructor
> OwnedPointer::new
> + unsafe { T::free_foreign(p) }
> + }
> +}
> diff --git a/qemu/src/util/mod.rs b/qemu/src/util/mod.rs
> new file mode 100644
> index 0000000..be00d0c
> --- /dev/null
> +++ b/qemu/src/util/mod.rs
> @@ -0,0 +1 @@
> +pub mod foreign;
> --
> 2.45.2
>
>
>
--
Marc-André Lureau
