Re: [PATCH v3 6/9] rust: workqueue: add helper for defining work_struct fields

[Alice Ryhl <aliceryhl@google.com>]

Benno Lossin <benno.lossin@proton.me> writes:
>> +/// Links for a work item.
>> +///
>> +/// This struct contains a function pointer to the `run` function from the [`WorkItemPointer`]
>> +/// trait, and defines the linked list pointers necessary to enqueue a work item in a workqueue.
>> +///
>> +/// Wraps the kernel's C `struct work_struct`.
>> +///
>> +/// This is a helper type used to associate a `work_struct` with the [`WorkItem`] that uses it.
>> +#[repr(transparent)]
>> +pub struct Work<T: ?Sized, const ID: u64 = 0> {
>> +    work: Opaque<bindings::work_struct>,
>> +    _inner: PhantomData<T>,
>
> Should this really be `PhantomData<T>`? Are you dropping `T`s in the 
> destructor of `Work<T>`? I do not think so `PhantomData<fn() -> Box<T>>`
> should do the trick.

Hmm. The `work_struct` effectively has a field of type `fn(T::Pointer)`,
so arguably, that's the type that should be used for the `PhantomData`.
This makes `Work` contravariant instead.

That said, the type `T` is going to be the struct itself, so, for
example, a `MyStruct<'a>` might have a `&'a i32` and a
`Work<MyStruct<'a>>` field, so it ends up being invariant.

By that argument, perhaps we should just make `Work` invariant instead?
We can do that with `PhantomData<*mut T>`.

>> +pub unsafe trait HasWork<T, const ID: u64 = 0> {
> 
> Is there a reason that `T` is `Sized` here?

Hmm, that's an oversight. In v1 of this patchset, the Sized requirement
was correct since T was the pointer type, but now it isn't anymore.

Alice