Re: [PATCH v8 3/8] rust: file: add Rust abstraction for `struct file`

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

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@proton.me>

---
Cheers,
Benno