Message-ID: <3dcd0379-b375-32d2-4956-c42afd451693@gmail.com>
Date:   Sat, 28 Jan 2023 17:46:50 -0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.0
Subject: Re: [PATCH 2/5] rust: types: introduce `ForeignOwnable`
Content-Language: en-US
To:     Boqun Feng <boqun.feng@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
References: <20230119174036.64046-2-wedsonaf@gmail.com>
 <20230128145345.1770912-1-yakoyoku@gmail.com>
 <Y9VV4+KCkShRex9m@boqun-archlinux>
From:   Martin Rodriguez Reboredo <yakoyoku@gmail.com>
In-Reply-To: <Y9VV4+KCkShRex9m@boqun-archlinux>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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