From: Alice Ryhl <aliceryhl@google.com>
To: boqun.feng@gmail.com
Cc: alex.gaynor@gmail.com, aliceryhl@google.com,
benno.lossin@proton.me, bjorn3_gh@protonmail.com,
gary@garyguo.net, linux-kernel@vger.kernel.org, ojeda@kernel.org,
patches@lists.linux.dev, rust-for-linux@vger.kernel.org,
wedsonaf@gmail.com
Subject: Re: [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut`
Date: Fri, 14 Jul 2023 10:28:00 +0000 [thread overview]
Message-ID: <20230714102800.2196236-1-aliceryhl@google.com> (raw)
In-Reply-To: <ZK2UeFE1A4iRfePS@boqun-archlinux>
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
next prev parent reply other threads:[~2023-07-14 10:28 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-10 7:46 [PATCH v1] rust: add improved version of `ForeignOwnable::borrow_mut` Alice Ryhl
2023-07-11 17:42 ` Boqun Feng
2023-07-14 10:28 ` Alice Ryhl [this message]
2023-07-13 3:28 ` Martin Rodriguez Reboredo
2023-07-15 13:38 ` Benno Lossin
2023-08-09 14:09 ` Andreas Hindborg (Samsung)
2023-08-22 9:31 ` Alice Ryhl
2023-08-23 8:40 ` Andreas Hindborg (Samsung)
2023-08-22 9:31 ` Alice Ryhl
2023-08-23 8:43 ` Andreas Hindborg (Samsung)
2023-08-23 10:34 ` Alice Ryhl
2023-08-23 16:09 ` Andreas Hindborg (Samsung)
2023-10-25 12:22 ` Ariel Miculas (amiculas)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230714102800.2196236-1-aliceryhl@google.com \
--to=aliceryhl@google.com \
--cc=alex.gaynor@gmail.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=gary@garyguo.net \
--cc=linux-kernel@vger.kernel.org \
--cc=ojeda@kernel.org \
--cc=patches@lists.linux.dev \
--cc=rust-for-linux@vger.kernel.org \
--cc=wedsonaf@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).