Message-ID: <CAH5fLgh3vrm6sH4dWejqRGEq6DCPHy-qViBybX4BwUWRbid55g@mail.gmail.com>
Date: Thu, 8 Feb 2024 16:06:00 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Martin Rodriguez Reboredo <yakoyoku@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...sung.com>, 
	Peter Zijlstra <peterz@...radead.org>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Arve Hjønnevåg <arve@...roid.com>, 
	Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>, 
	Joel Fernandes <joel@...lfernandes.org>, Carlos Llamas <cmllamas@...gle.com>, 
	Suren Baghdasaryan <surenb@...gle.com>, Dan Williams <dan.j.williams@...el.com>, 
	Kees Cook <keescook@...omium.org>, Matthew Wilcox <willy@...radead.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Daniel Xu <dxu@...uu.xyz>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v4 3/9] rust: file: add Rust abstraction for `struct file`

On Thu, Feb 8, 2024 at 1:35 AM Martin Rodriguez Reboredo
<yakoyoku@...il.com> wrote:
>
> On 2/2/24 07:55, Alice Ryhl wrote:
> > From: Wedson Almeida Filho <wedsonaf@...il.com>
> >
> > This abstraction makes it possible to manipulate the open files for a
> > process. The new `File` struct wraps the C `struct file`. When accessing
> > it using the smart pointer `ARef<File>`, the pointer will own a
> > reference count to the file. When accessing it as `&File`, then the
> > reference does not own a refcount, but the borrow checker will ensure
> > that the reference count does not hit zero while the `&File` is live.
> >
> > Since this is intended to manipulate the open files of a process, we
> > introduce an `fget` constructor that corresponds to the C `fget`
> > method. In future patches, it will become possible to create a new fd in
> > a process and bind it to a `File`. Rust Binder will use these to send
> > fds from one process to another.
> >
> > We also provide a method for accessing the file's flags. Rust Binder
> > will use this to access the flags of the Binder fd to check whether the
> > non-blocking flag is set, which affects what the Binder ioctl does.
> >
> > This introduces a struct for the EBADF error type, rather than just
> > using the Error type directly. This has two advantages:
> > * `File::from_fd` returns a `Result<ARef<File>, BadFdError>`, which the
> >    compiler will represent as a single pointer, with null being an error.
> >    This is possible because the compiler understands that `BadFdError`
> >    has only one possible value, and it also understands that the
> >    `ARef<File>` smart pointer is guaranteed non-null.
> > * Additionally, we promise to users of the method that the method can
> >    only fail with EBADF, which means that they can rely on this promise
> >    without having to inspect its implementation.
> > That said, there are also two disadvantages:
> > * Defining additional error types involves boilerplate.
> > * The question mark operator will only utilize the `From` trait once,
> >    which prevents you from using the question mark operator on
> >    `BadFdError` in methods that return some third error type that the
> >    kernel `Error` is convertible into. (However, it works fine in methods
> >    that return `Error`.)
> >
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > Co-developed-by: Daniel Xu <dxu@...uu.xyz>
> > Signed-off-by: Daniel Xu <dxu@...uu.xyz>
> > Co-developed-by: Alice Ryhl <aliceryhl@...gle.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> > ---
> > [...]
> > +/// ## Rust references
> > +///
> > +/// The reference type `&File` is similar to light refcounts:
> > +///
> > +/// * `&File` references don't own a reference count. They can only exist as long as the reference
> > +///   count stays positive, and can only be created when there is some mechanism in place to ensure
> > +///   this.
> > +///
> > +/// * The Rust borrow-checker normally ensures this by enforcing that the `ARef<File>` from which
> > +///   a `&File` is created outlives the `&File`.
> > +///
> > +/// * Using the unsafe [`File::from_ptr`] means that it is up to the caller to ensure that the
> > +///   `&File` only exists while the reference count is positive.
> > +///
> > +/// * You can think of `fdget` as using an fd to look up an `ARef<File>` in the `struct
> > +///   files_struct` and create an `&File` from it. The "fd cannot be closed" rule is like the Rust
> > +///   rule "the `ARef<File>` must outlive the `&File`".
>
> I find it kinda odd that this unordered list interspaces elements with
> blank lines as opposed to the following one, though, I don't see it as
> rather a big deal.

Shrug. I did it here because I found it more readable.

> > +///
> > +/// # Invariants
> > +///
> > +/// * Instances of this type are refcounted using the `f_count` field.
> > +/// * If an fd with active light refcounts is closed, then it must be the case that the file
> > +///   refcount is positive until all light refcounts of the fd have been dropped.
> > +/// * A light refcount must be dropped before returning to userspace.
> > [...]
>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@...il.com>

Powered by blists - more mailing lists