Re: [PATCH 7/7] rust: file: add abstraction for `poll_table`

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

On 11/29/23 14:12, Alice Ryhl wrote:
> diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
> index 578ee307093f..35576678c993 100644
> --- a/rust/kernel/file.rs
> +++ b/rust/kernel/file.rs
> @@ -14,6 +14,9 @@
>  use alloc::boxed::Box;
>  use core::{alloc::AllocError, marker::PhantomData, mem, ptr};
> 
> +mod poll_table;
> +pub use self::poll_table::{PollCondVar, PollTable};

I think it makes more sense to put it under `rust/kernel/sync/`.
> +    fn get_qproc(&self) -> bindings::poll_queue_proc {
> +        let ptr = self.0.get();
> +        // SAFETY: The `ptr` is valid because it originates from a reference, and the `_qproc`
> +        // field is not modified concurrently with this call.

What ensures this? Maybe use a type invariant?

> +        unsafe { (*ptr)._qproc }
> +    }

[...]

> +impl PollCondVar {
> +    /// Constructs a new condvar initialiser.
> +    #[allow(clippy::new_ret_no_self)]

This is no longer needed, as Gary fixed this, see [1].

[1]: https://github.com/rust-lang/rust-clippy/issues/7344

> +    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
> +        pin_init!(Self {
> +            inner <- CondVar::new(name, key),
> +        })
> +    }
> +}
> +
> +// Make the `CondVar` methods callable on `PollCondVar`.
> +impl Deref for PollCondVar {
> +    type Target = CondVar;
> +
> +    fn deref(&self) -> &CondVar {
> +        &self.inner
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for PollCondVar {
> +    fn drop(self: Pin<&mut Self>) {
> +        // Clear anything registered using `register_wait`.
> +        self.inner.notify(1, bindings::POLLHUP | bindings::POLLFREE);

Isn't notifying only a single thread problematic, since a user could
misuse the `PollCondVar` (since all functions of `CondVar` are also
accessible) and also `.wait()` on the condvar? When dropping a
`PollCondVar` it might notify only the user `.wait()`, but not the
`PollTable`. Or am I missing something?

-- 
Cheers,
Benno

> +        // Wait for epoll items to be properly removed.
> +        //
> +        // SAFETY: Just an FFI call.
> +        unsafe { bindings::synchronize_rcu() };
> +    }
> +}