Re: [PATCH v4 09/28] rust: alloc: implement kernel `Box`

[Benno Lossin <benno.lossin@proton.me>]

On 07.08.24 12:38, Danilo Krummrich wrote:
> On Wed, Aug 07, 2024 at 07:49:31AM +0000, Benno Lossin wrote:
>>>>> +    {
>>>>> +        Ok(Self::new(x, flags)?.into())
>>>>> +    }
>>>>> +
>>>>> +    /// Drops the contents, but keeps the allocation.
>>>>> +    ///
>>>>> +    /// # Examples
>>>>> +    ///
>>>>> +    /// ```
>>>>> +    /// let value = KBox::new([0; 32], GFP_KERNEL)?;
>>>>> +    /// assert_eq!(*value, [0; 32]);
>>>>> +    /// let value = KBox::drop_contents(value);
>>>>> +    /// // Now we can re-use `value`:
>>>>> +    /// let value = KBox::write(value, [1; 32]);
>>>>> +    /// assert_eq!(*value, [1; 32]);
>>>>> +    /// # Ok::<(), Error>(())
>>>>> +    /// ```
>>>>> +    pub fn drop_contents(this: Self) -> Box<MaybeUninit<T>, A> {
>>>>> +        let ptr = Box::into_raw(this);
>>>>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>>>> +        unsafe { core::ptr::drop_in_place(ptr) };
>>>>> +        // SAFETY: `ptr` is valid, because it came from `Box::into_raw`.
>>>>> +        unsafe { Box::from_raw(ptr.cast()) }
>>>>> +    }
>>>>
>>>> I don't particularly care in this instance, but you just took my patch
>>>> and folded it into your own without asking me or specifying it in the
>>>> commit message. In general I would have assumed that you just put the
>>>> entire patch into the series (with correct From:... etc).
>>>
>>> When I asked about this in [1] my understanding was that we expect [1] to land
>>> prior to this series. So, I'm just anticipating a future rebase where I move
>>> this code from box_ext.rs to kbox.rs, just like Alice suggested for her
>>> "ForeignOwnable for Pin<crate::alloc::Box<T, A>>" implementation.
>>>
>>> I also understand your later reply, where you said: "[...] then you can just
>>> include it when you remove the `BoxExit` trait." as confirmation.
>>>
>>> Probably that's a misunderstanding though. Sorry if that's the case.
>>
>> Yeah what I meant by that was you base it on top and then move it from
>> the `BoxExt` trait over to `Box` in a correctly attributed patch.
> 
> I don't understand this. What do you mean with "correctly attributed patch" in
> this case?

So, seems like I mixed the two approaches I thought of. Here are they
separated:
1. base the series on top of my patches and then copy the implementation
   from the in-tree file this patch.
2. create a new patch that adds `drop_contents` to your Box (ie after
   this patch) that has `Signed-off-by: you` and `Co-Developed-by: me`.
   With this approach I would also ask privately (including the patch)
   if that would be ok.

---
Cheers,
Benno

> There are various existing implementations around `Box` and `BoxExt`, are you
> saying that I should create separate patches for moving / adapting all of them,
> e.g. this patch adapts parts from `BoxExt`, the implementation of
> `ForeignOwnable` for `Box<T>`, the implementation of `InPlaceInit<T>` for
> `Box<T>`.
> 
> I don't think this is necessary.
> 
> I probably shouldn't anticipate a future rebase though, it just leads to
> confusion. I'll drop it for now and re-add it once your patch lands in rust-next.
> 
>> As I
>> said above, I don't really mind in this case, since it's trivial, so no
>> worries. Just a heads-up for occasions where it is non-trivial.
>>
>>> [1] https://lore.kernel.org/lkml/24a8d381-dd13-4d19-a736-689b8880dbe1@proton.me/