On 25.07.24 16:27, Alice Ryhl wrote:
> +/// Wraps the kernel's `struct file`. Not thread safe.
> +///
> +/// This type represents a file that is not known to be safe to transfer across thread boundaries.
> +/// To obtain a thread-safe [`File`], use the [`assume_no_fdget_pos`] conversion.
> +///
> +/// See the documentation for [`File`] for more information.
> +///
> +/// # Invariants
> +///
> +/// * All instances of this type are refcounted using the `f_count` field.
> +/// * If there is an active call to `fdget_pos` that did not take the `f_pos_lock` mutex, then it
> +/// must be on the same thread as this `File`.
Do you mean `LocalFile`?
> +///
> +/// [`assume_no_fdget_pos`]: LocalFile::assume_no_fdget_pos
> +pub struct LocalFile {
> + inner: Opaque<bindings::file>,
> +}
[...]
> + /// Returns the flags associated with the file.
> + ///
> + /// The flags are a combination of the constants in [`flags`].
> + #[inline]
> + pub fn flags(&self) -> u32 {
> + // This `read_volatile` is intended to correspond to a READ_ONCE call.
> + //
> + // SAFETY: The file is valid because the shared reference guarantees a nonzero refcount.
> + //
> + // FIXME(read_once): Replace with `read_once` when available on the Rust side.
Do you know the status of this?
> + unsafe { core::ptr::addr_of!((*self.as_ptr()).f_flags).read_volatile() }
> + }
> +}
> +
> +impl File {
> + /// Creates a reference to a [`File`] from a valid pointer.
> + ///
> + /// # Safety
> + ///
> + /// * The caller must ensure that `ptr` points at a valid file and that the file's refcount is
> + /// positive for the duration of 'a.
> + /// * The caller must ensure that if there are active `fdget_pos` calls on this file, then they
> + /// took the `f_pos_lock` mutex.
> + #[inline]
> + pub unsafe fn from_raw_file<'a>(ptr: *const bindings::file) -> &'a File {
> + // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
> + // duration of 'a. The cast is okay because `File` is `repr(transparent)`.
> + //
> + // INVARIANT: The caller guarantees that there are no problematic `fdget_pos` calls.
> + unsafe { &*ptr.cast() }
> + }
> +}
> +
> +// Make LocalFile methods available on File.
> +impl core::ops::Deref for File {
> + type Target = LocalFile;
> + #[inline]
> + fn deref(&self) -> &LocalFile {
> + // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a
> + // valid file for the desired duration.
> + //
> + // By the type invariants, there are no `fdget_pos` calls that did not take the
> + // `f_pos_lock` mutex.
> + unsafe { LocalFile::from_raw_file(self as *const File as *const bindings::file) }
> + }
> +}
> +
> +// SAFETY: The type invariants guarantee that `LocalFile` is always ref-counted. This implementation
> +// makes `ARef<File>` own a normal refcount.
> +unsafe impl AlwaysRefCounted for LocalFile {
> + #[inline]
> + fn inc_ref(&self) {
> + // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> + unsafe { bindings::get_file(self.as_ptr()) };
> + }
> +
> + #[inline]
> + unsafe fn dec_ref(obj: ptr::NonNull<LocalFile>) {
> + // SAFETY: To call this method, the caller passes us ownership of a normal refcount, so we
> + // may drop it. The cast is okay since `File` has the same representation as `struct file`.
> + unsafe { bindings::fput(obj.cast().as_ptr()) }
> + }
> +}
Can you move these `AlwaysRefCounted` impls towards the struct
definitions?
With those two comments fixed:
Reviewed-by: Benno Lossin <benno.lossin@xxxxxxxxx>
---
Cheers,
Benno
