Messages in this thread | | | Date | Fri, 14 Jul 2023 10:28:00 +0000 | Subject | Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut` | From | Alice Ryhl <> |
| |
Boqun Feng <boqun.feng@gmail.com> writes: > On Mon, Jul 10, 2023 at 07:46:42AM +0000, Alice Ryhl wrote: >> Previously, the `ForeignOwnable` trait had a method called `borrow_mut` >> that was intended to provide mutable access to the inner value. However, >> the method accidentally made it possible to change the address of the >> object being modified, which usually isn't what we want. (And when we >> want that, it can be done by calling `from_foreign` and `into_foreign`, >> like how the old `borrow_mut` was implemented.) >> >> In this patch, we introduce an alternate definition of `borrow_mut` that >> solves the previous problem. Conceptually, given a pointer type `P` that >> implements `ForeignOwnable`, the `borrow_mut` method gives you the same >> kind of access as an `&mut P` would, except that it does not let you >> change the pointer `P` itself. >> >> This is analogous to how the existing `borrow` method provides the same >> kind of access to the inner value as an `&P`. >> >> Note that for types like `Arc`, having an `&mut Arc<T>` only gives you >> immutable access to the inner `T`. This is because mutable references >> assume exclusive access, but there might be other handles to the same >> reference counted value, so the access isn't exclusive. The `Arc` type >> implements this by making `borrow_mut` return the same type as `borrow`. >> >> Signed-off-by: Alice Ryhl <aliceryhl@google.com> >> --- >> >> This patch depends on https://lore.kernel.org/all/20230706094615.3080784-1-aliceryhl@google.com/ >> >> rust/kernel/sync/arc.rs | 31 +++++++++----- >> rust/kernel/types.rs | 93 ++++++++++++++++++++++++++++++----------- >> 2 files changed, 89 insertions(+), 35 deletions(-) >> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs >> index d479f8da8f38..1c2fb36906b6 100644 >> --- a/rust/kernel/types.rs >> +++ b/rust/kernel/types.rs >> @@ -20,66 +20,111 @@ >> /// This trait is meant to be used in cases when Rust objects are stored in C objects and >> /// eventually "freed" back to Rust. >> pub trait ForeignOwnable: Sized { >> - /// Type of values borrowed between calls to [`ForeignOwnable::into_foreign`] and >> - /// [`ForeignOwnable::from_foreign`]. >> + /// Type used to immutably borrow a value that is currently foreign-owned. >> type Borrowed<'a>; >> >> + /// Type used to mutably borrow a value that is currently foreign-owned. >> + type BorrowedMut<'a>; >> + > > I would probably want to say "if the `impl ForeignOwnable` doesn't have > the exclusive ownership, `BorrowedMut` should be the same as `Borrowed`" > for logical self-consistent,
I don't phrase it as a requirement, but I do already say something along these lines.
> and even further make it as default as follow: > > type BorrowedMut<'a> = Self::Borrowed<'a>; > > unsafe fn borrow_mut<'a>(ptr: *const core::ffi::c_void) -> Self::BorrowedMut<'a> { > Self::borrow(ptr) > } > > but it might be over-engineering (and require associated_type_defaults > and more)...
I don't think it makes sense to use defaults here. There are very few implementers of this trait, and I'd like all of them to explicitly think about whether `BorrowedMut` should be the same as `Borrowed`, or different.
> Anyway, > > Reviewed-by: Boqun Feng <boqun.feng@gmail.com> > > Regards, > Boqun
Alice
| |