Messages in this thread | | | From | "Andreas Hindborg (Samsung)" <> | Subject | Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut` | Date | Wed, 23 Aug 2023 10:43:18 +0200 |
| |
Alice Ryhl <aliceryhl@google.com> writes:
> Andreas Hindborg <nmi@metaspace.dk> 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.
I don't see how that prevents this trait from being split in two?
BR Andreas
| |