Re: [PATCH v1 5/7] rust: workqueue: add helper for defining work_struct fields

[Andreas Hindborg]

Alice Ryhl <aliceryhl@xxxxxxxxxx> writes:

> Andreas Hindborg <nmi@xxxxxxxxxxxx> writes:
>> Alice Ryhl <aliceryhl@xxxxxxxxxx> writes:
>>> +/// Used to safely implement the [`HasWork<T>`] trait.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```
>>> +/// use kernel::sync::Arc;
>>> +///
>>> +/// struct MyStruct {
>>> +///     work_field: Work<Arc<MyStruct>>,
>>> +/// }
>>> +///
>>> +/// impl_has_work! {
>>> +///     impl HasWork<Arc<MyStruct>> for MyStruct { self.work_field }
>>> +/// }
>>> +/// ```
>>> +///
>>> +/// [`HasWork<T>`]: HasWork
>>> +#[macro_export]
>>> +macro_rules! impl_has_work {
>>> +    ($(impl$(<$($implarg:ident),*>)?
>>> +       HasWork<$work_type:ty>
>>> +       for $self:ident $(<$($selfarg:ident),*>)?
>>> +       { self.$field:ident }
>>> +    )*) => {$(
>>> +        // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
>>> +        // type.
>>> +        unsafe impl$(<$($implarg),*>)? $crate::workqueue::HasWork<$work_type> for $self $(<$($selfarg),*>)? {
>>> +            const OFFSET: usize = $crate::offset_of!(Self, $field) as usize;
>>> +
>>> +            #[inline]
>>> +            unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type> {
>>> +                // SAFETY: The caller promises that the pointer is not dangling.
>>> +                unsafe {
>>> +                    ::core::ptr::addr_of_mut!((*ptr).$field)
>>> +                }
>>> +            }
>> 
>> What is the reason for overriding the default implementation of `raw_get_work()`?
>> 
>> BR Andreas
>
> That's how the macro checks that the field actually has the type you
> claim it has. If you lie about the type, then `raw_get_work` will not
> compile. (See the safety comment on the impl block.)

Got it 👍

I was thinking we could do the type check without redefining the method,
but that blows up complexity wise fast, since we need a trait to do it
to support `Self` in `$work_type`. It strikes me as a bit of a hack to
overwrite an otherwise fine implementation, but I guess it is the least
complex way.

Also I am a bit annoyed that we need to state the `$work_type` type at
all, since it is available in `work_field`. But I can see no way around
that.

BR Andreas