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() };
> + }
> +}
