Feedback-ID: iad51458e:Fastmail
Date:   Sat, 28 Jan 2023 14:07:02 -0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Cc:     wedsonaf@gmail.com, alex.gaynor@gmail.com,
        bjorn3_gh@protonmail.com, gary@garyguo.net,
        linux-kernel@vger.kernel.org, ojeda@kernel.org,
        rust-for-linux@vger.kernel.org
Subject: Re: [PATCH 2/5] rust: types: introduce `ForeignOwnable`
Message-ID: <Y9WchuzdL8HBrqq7@boqun-archlinux>
References: <20230119174036.64046-2-wedsonaf@gmail.com>
 <20230128145345.1770912-1-yakoyoku@gmail.com>
 <Y9VV4+KCkShRex9m@boqun-archlinux>
 <3dcd0379-b375-32d2-4956-c42afd451693@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3dcd0379-b375-32d2-4956-c42afd451693@gmail.com>
Precedence: bulk

On Sat, Jan 28, 2023 at 05:46:50PM -0300, Martin Rodriguez Reboredo wrote:
> On 1/28/23 14:05, Boqun Feng wrote:
> > On Sat, Jan 28, 2023 at 11:53:45AM -0300, Martin Rodriguez Reboredo wrote:
> > [...]
> >>> +    /// Borrows a foreign-owned object.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
> >>> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
> >>> +    /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow_mut`]
> >>> +    /// for this object must have been dropped.
> >>> +    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> Self::Borrowed<'a>;
> >>> +
> >>> +    /// Mutably borrows a foreign-owned object.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
> >>> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
> >>> +    /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] and
> >>> +    /// [`ForeignOwnable::borrow_mut`] for this object must have been dropped.
> >>> +    unsafe fn borrow_mut<T: ForeignOwnable>(ptr: *const core::ffi::c_void) -> ScopeGuard<T, fn(T)> {
> >>> +        // SAFETY: The safety requirements ensure that `ptr` came from a previous call to
> >>> +        // `into_foreign`.
> >>> +        ScopeGuard::new_with_data(unsafe { T::from_foreign(ptr) }, |d| {
> >>> +            d.into_foreign();
> >>> +        })
> >>> +    }
> >>
> >> Could these three methods have a borrowing equivalent? When I was
> >> working on some features for the USB module I've stumbled upon the case
> >> of having to encode a pointer (with a pivot) and I cannot do it without
> >> taking ownership of the pointer.
> >>
> > 
> > *const T is Copy, so you can still use it after pass it to a function or
> > a new binding, e.g.
> > 
> > 	pub fn use_ptr(ptr: *const i32) { .. }
> > 
> > 	let p: *const i32 = some_func();
> > 
> > 	let q = p;
> > 
> > 	// q is just a copy of p
> > 	use_ptr(p);
> > 	// passing to a function parameter is just copying
> > 	use_ptr(p);
> > 
> > maybe I'm missing something subtle, but if you have an example I can
> > help take a look.
> > 
> > Regards,
> > Boqun
> > 
> 
> I'll use a much more simple example. If I want to take the byte offset
> between two `ForeignWrapper`s I'd have to take ownership of them, but I
> don't see it desirable in some cases.
> 
>     fn byte_offset<P: PointerWrapper>(ptr: P, pivot: P) -> isize {
>         unsafe {
>             ptr.into_pointer().cast::<u8>()
>                 .byte_offset(pivot.into_pointer().cast())
>         }
>     }
> 
> But if there was an `as_pointer(&self) -> *const c_void` method then the
> above function will be able to borrow both `ForeignWrapper`s.
> 
>     fn byte_offset<P: PointerWrapper>(ptr: &P, pivot: &P) -> isize {
>         unsafe {
>             ptr.as_pointer().cast::<u8>()
>                 .byte_offset(pivot.as_pointer().cast())
>         }
>     }
> 
> Obviously those methods that borrow will announce invariancies in their
> doc comments. If these can exist then great and if not then another
> solution could be explored.
> 

For this particular use case, IIUC, what you need is exactly `AsRef`:

     fn byte_offset<T, P: AsRef<T>>(ptr: &P, pivot: &P) -> isize {
         unsafe {
             (ptr.as_ref() as *const _).cast::<u8>()
                 .byte_offset((pivot.as_ref() *const _).cast())
         }
     }

or you can implement a as_pointer() helper function to avoid the
duplicate `.as_ref() as *const _`:

    fn as_pointer<T, P: AsRef<T>>(ptr: &P) -> *const T {
        ptr.as_ref() as *const T
    }

Although, we need to `impl AsRef<T> for Arc<T>` to make it work for
`Arc<T>`, which is currently missing.

Regards,
Boqun

> >>> +
> >>> +    /// Converts a foreign-owned object back to a Rust-owned one.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// `ptr` must have been returned by a previous call to [`ForeignOwnable::into_foreign`] for
> >>> +    /// which a previous matching [`ForeignOwnable::from_foreign`] hasn't been called yet.
> >>> +    /// Additionally, all instances (if any) of values returned by [`ForeignOwnable::borrow`] and
> >>> +    /// [`ForeignOwnable::borrow_mut`] for this object must have been dropped.
> >>> +    unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self;
> >>> +}
> >>> +
> >>>  /// Runs a cleanup function/closure when dropped.
> >>>  ///
> >>>  /// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.
> >>> -- 
> >>> 2.34.1
> >>
> >> Aside from these comments I observe that there's a possibility to make
> >> ForeignOwnable a const trait and have non const implementors. Otherwise
> >> if these things are out of scope, no problem whatsoever and this has my
> >> OK.
> >>
> >> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>