Feedback-ID: iad51458e:Fastmail
Date:   Sat, 28 Jan 2023 09:05:39 -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: <Y9VV4+KCkShRex9m@boqun-archlinux>
References: <20230119174036.64046-2-wedsonaf@gmail.com>
 <20230128145345.1770912-1-yakoyoku@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230128145345.1770912-1-yakoyoku@gmail.com>
Precedence: bulk

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

> > +
> > +    /// 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>