On Tue, Aug 6, 2024 at 10:44 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>
> 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`?
I guess. Perhaps I should just say "file" as a general concept?
> > +///
> > +/// [`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?
It's still unavailable.
> > + 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>
Thanks!
Alice
