Hi

On Fri, Sep 11, 2020 at 5:08 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 11/09/20 12:24, Paolo Bonzini wrote:
> >>
> >> - from_qemu_none(ptr: *const sys::P) -> T
> >>   Return a Rust type T for a const ffi pointer P.
> >>
> >> - from_qemu_full(ptr: *mut sys::P) -> T
> >>   Return a Rust type T for a ffi pointer P, taking ownership.
> >>
> >> - T::to_qemu_none() -> Stash<P>
> >>   Returns a borrowed ffi pointer P (using a Stash to destroy "glue"
> >>   storage data, if any).
> >>
> >> - T::to_qemu_full() -> P
> >>   Returns a ffi pointer P. (P resources are leaked/passed to C/ffi)
> >
> > I know these come from glib-rs, but still the names are awful. :)
>
> After studying a bit I managed to give a rational explanation of the
> above gut feeling.  Conversions in Rust usually follow this naming
> convention:
>
>         Name              Type                  Price
>         --------          --------------------  -----------
>         as_*, borrow      Borrowed -> Borrowed  Cheap
>         to_*, constructor Borrowed -> Owned     Expensive
>         from, into_*      Owned -> Owned        Any
>
> and we have
>
>         from_qemu_none    Borrowed -> Owned
>         to_qemu_none      Borrowed -> Borrowed
>         from_qemu_full    Owned -> Owned
>         to_qemu_full      Owned -> Owned
>
> So
>
> - from_qemu_none should be a "to_*" or "constructor" conversion (I used
> new_from_foreign)
>

new_ prefix is not very rusty.

from_raw() is common, and takes ownership.
from_ptr() (as in CStr) could be a candidate (although it wouldn't have the
same lifetime requirements)


> - to_qemu_none, even though in some cases it can be expensive, should be
> an "as_*" conversion (as_foreign).
>

as_ptr(), but since it's more complicated than just a pointer due to
temporary storage, it's not the best fit either...


> - from_qemu_full and to_qemu_full should be "from" or "into_*"
>

ok


>
> to_qemu_full() could also be split into a more expensive but flexible
> "to" variant and the cheaper "into" variant.  Just for the sake of
> example, I updated my demo by replacing the IntoRaw trait with these two:
>
>     trait ToForeign<T> {
>         fn to_foreign(&self) -> *mut T;
>     }
>
>     trait IntoForeign<T> {
>         fn into_foreign(self) -> *mut T;
>     }
>
> where the example implementations show the different cost quite clearly:
>
>     impl ToForeign<c_char> for String {
>         fn to_foreign(&self) -> *mut c_char {
>             unsafe { libc::strndup(self.as_ptr() as *const c_char,
>                                    self.len() as size_t) }
>         }
>     }
>
>     impl IntoForeign<c_char> for String {
>         fn into_foreign(self) -> *mut c_char {
>             let ptr = self.as_ptr();
>             forget(self);
>             ptr as *mut _
>         }
>     }
>
>
You corrected the \0-ending in the git tree. However, the memory allocator
(or the stack) may not be compatible with the one used in C.

 As for the general comparison with glib-rs traits, it's hard for me to say
upfront the pros and cons, I would need to modify this PoC for example. 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.

(my gut feeling is that we would be better sticking with something close to
glib, as it is likely we will end up using glib-rs at some point, and
having similar patterns should help each other)