Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut`

[Alice Ryhl]

Andreas Hindborg <nmi@xxxxxxxxxxxx> writes:
>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>> index 172f563976a9..f152a562c9c3 100644
>> --- a/rust/kernel/sync/arc.rs
>> +++ b/rust/kernel/sync/arc.rs
>> @@ -232,26 +232,35 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
>>  
>>  impl<T: 'static> ForeignOwnable for Arc<T> {
>>      type Borrowed<'a> = ArcBorrow<'a, T>;
>> +    // Mutable access to the `Arc` does not give any extra abilities over
>> +    // immutable access.
>> +    type BorrowedMut<'a> = ArcBorrow<'a, T>;
>>  
>>      fn into_foreign(self) -> *const core::ffi::c_void {
>>          ManuallyDrop::new(self).ptr.as_ptr() as _
>>      }
>>  
>> -    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
>> -        // SAFETY: By the safety requirement of this function, we know that `ptr` came from
>> -        // a previous call to `Arc::into_foreign`.
>> -        let inner = NonNull::new(ptr as *mut ArcInner<T>).unwrap();
>> -
>> -        // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
>> -        // for the lifetime of the returned value.
>> -        unsafe { ArcBorrow::new(inner) }
>> -    }
>> -
>>      unsafe fn from_foreign(ptr: *const core::ffi::c_void) -> Self {
>>          // SAFETY: By the safety requirement of this function, we know that `ptr` came from
>>          // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
>>          // holds a reference count increment that is transferrable to us.
>> -        unsafe { Self::from_inner(NonNull::new(ptr as _).unwrap()) }
>> +        unsafe { Self::from_inner(NonNull::new_unchecked(ptr as _)) }
>>      }
>> +
>> +    unsafe fn borrow<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
>> +        // SAFETY: By the safety requirement of this function, we know that `ptr` came from
>> +        // a previous call to `Arc::into_foreign`.
>> +        let inner = unsafe { NonNull::new_unchecked(ptr as *mut ArcInner<T>) };
>> +
>> +        // SAFETY: The safety requirements ensure that we will not give up our
>> +        // foreign-owned refcount while the `ArcBorrow` is still live.
>> +        unsafe { ArcBorrow::new(inner) }
>> +    }
>> +
>> +    unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> ArcBorrow<'a, T> {
>> +        // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
>> +        // requirements for `borrow`.
>> +        unsafe { Self::borrow(ptr) }
>> +    }
>
> I am not sure this makes sense. How about splitting the trait in two,
> immutable and mutable and only implementing the immutable one or Arc?

I used this design based on what would make sense for a linked list. The
idea is that we can have two different types of cursors for a linked
list: immutable and mutable. The immutable cursor lets you:

 * move around the linked list
 * access the values using `borrow`

The mutable cursor lets you:

 * move around the linked list
 * delete or add items to the list
 * access the values using `borrow_mut`

The mutable cursor gives you extra abilities beyond the `borrow` vs
`borrow_mut` distinction, so we want to provide both types of cursors
even if the pointer type is Arc. To do that, we need a trait that
defines what it means to have mutable access to an Arc.

Alice