Messages in this thread | | | Date | Tue, 23 May 2023 11:07:09 +0000 | Subject | Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue | From | Alice Ryhl <> |
| |
On 5/18/23 21:17, Martin Rodriguez Reboredo wrote: > On 5/17/23 17:31, Alice Ryhl wrote: >> +unsafe impl<T> WorkItem for Arc<T> >> +where >> + T: ArcWorkItem + HasWork<Self> + ?Sized, >> +{ >> + type EnqueueOutput = Result<(), Self>; >> + >> + unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput >> + where >> + F: FnOnce(*mut bindings::work_struct) -> bool, >> + { >> + let ptr = Arc::into_raw(self); >> + >> + // Using `get_work_offset` here for object-safety. >> + // >> + // SAFETY: The pointer is valid since we just got it from `into_raw`. >> + let off = unsafe { (&*ptr).get_work_offset() }; >> + >> + // SAFETY: The `HasWork` impl promises that this offset gives us a field of type >> + // `Work<Self>` in the same allocation. >> + let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work<Self> }; >> + // SAFETY: The pointer is not dangling. >> + let work_ptr = unsafe { Work::raw_get(work_ptr) }; >> + >> + match (queue_work_on)(work_ptr) { > > Match for boolean is not a good pattern in my eyes, if-else should be > used instead.
I think this is a question of style. For a comparison:
match (queue_work_on)(work_ptr) { true => Ok(()), // SAFETY: The work queue has not taken ownership of the pointer. false => Err(unsafe { Arc::from_raw(ptr) }), }
vs
if (queue_work_on)(work_ptr) { Ok(()) } else { // SAFETY: The work queue has not taken ownership of the pointer. Err(unsafe { Arc::from_raw(ptr) }), }
I'm happy to change it if others disagree, but when the branches evaluate to a short expression like they do here, I quite like the first version.
> Also aren't the parens around the closure unnecessary?
Hmm, parenthesises are often required around closures, but it's possible that it is only required for stuff like `self.closure(args)` to disambiguate between a `closure` field (of pointer type) and a `closure` method. I can check and remove them if they are not necessary.
| |