Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Jul 2023 14:08:44 +0000
To:     Martin Rodriguez Reboredo <yakoyoku@gmail.com>,
        Miguel Ojeda <ojeda@kernel.org>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Alex Gaynor <alex.gaynor@gmail.com>
From:   Benno Lossin <benno.lossin@proton.me>
Cc:     Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
        =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
        Alice Ryhl <aliceryhl@google.com>,
        Andreas Hindborg <nmi@metaspace.dk>,
        rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
        Asahi Lina <lina@asahilina.net>
Subject: Re: [PATCH v2 11/12] rust: init: add `{pin_}chain` functions to `{Pin}Init<T, E>`
Message-ID: <5f22b25d-132d-7cbc-8bca-8333516c1663@proton.me>
In-Reply-To: <0b818707-4762-c12d-8624-7d3c4f6841da@gmail.com>
References: <20230719141918.543938-1-benno.lossin@proton.me> <20230719141918.543938-12-benno.lossin@proton.me> <0b818707-4762-c12d-8624-7d3c4f6841da@gmail.com>
Feedback-ID: 71780778:user:proton
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 21.07.23 02:23, Martin Rodriguez Reboredo wrote:
> On 7/19/23 11:21, Benno Lossin wrote:
>> +/// An initializer returned by [`PinInit::pin_chain`].
>> +pub struct ChainPinInit<I, F, T: ?Sized, E>(I, F, __internal::Invariant=
<(E, Box<T>)>);
>> +
>> +// SAFETY: the `__pinned_init` function is implemented such that it
>> +// - returns `Ok(())` on successful initialization,
>> +// - returns `Err(err)` on error and in this case `slot` will be droppe=
d.
>> +// - considers `slot` pinned.
>> +unsafe impl<T: ?Sized, E, I, F> PinInit<T, E> for ChainPinInit<I, F, T,=
 E>
>> +where
>> +    I: PinInit<T, E>,
>> +    F: FnOnce(Pin<&mut T>) -> Result<(), E>,
>> +{
>> +    unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
>> +        // SAFETY: all requirements fulfilled since this function is `_=
_pinned_init`.
>> +        unsafe { self.0.__pinned_init(slot)? };
>> +        // SAFETY: The above call initialized `slot` and we still have =
unique access.
>> +        let val =3D unsafe { &mut *slot };
>> +        // SAFETY: `slot` is considered pinned
>> +        let val =3D unsafe { Pin::new_unchecked(val) };
>> +        (self.1)(val).map_err(|e| {
>> +            // SAFETY: `slot` was initialized above.
>> +            unsafe { core::ptr::drop_in_place(slot) };
>> +            e
>=20
> I might stumble upon an error like EAGAIN if I call `pin_chain` but that
> means `slot` will be dropped. So my recommendation is to either not drop
> the value or detail in `pin_chain`'s doc comment that the closure will
> drop on error.

This is a bit confusing to me, because dropping the value on returning `Err=
`
is a safety requirement of `PinInit`. Could you elaborate why this is
surprising? I can of course add it to the documentation, but I do not see
how it could be implemented differently. Since if you do not drop the value
here, nobody would know that it is still initialized.

--=20
Cheers,
Benno