Re: [PATCH 2/3] rust: file: add f_pos and set_f_pos

Christian Brauner
2024-09-27T07:32:08Z

On Thu, Sep 26, 2024 at 02:58:56PM GMT, Alice Ryhl wrote:
> Add accessors for the file position. Most of the time, you should not
> use these methods directly, and you should instead use a guard for the
> file position to prove that you hold the fpos lock. However, under
> limited circumstances, files are allowed to choose a different locking
> strategy for their file position. These accessors can be used to handle
> that case.
>
> For now, these accessors are the only way to access the file position
> within the llseek and read_iter callbacks.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/fs/file.rs | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
> index e03dbe14d62a..c896a3b1d182 100644
> --- a/rust/kernel/fs/file.rs
> +++ b/rust/kernel/fs/file.rs
> @@ -333,6 +333,26 @@ pub fn flags(&self) -> u32 {
> // FIXME(read_once): Replace with `read_once` when available on the Rust side.
> unsafe { core::ptr::addr_of!((*self.as_ptr()).f_flags).read_volatile() }
> }
> +
> + /// Read the file position.
> + ///
> + /// # Safety
> + ///
> + /// You must hold the fpos lock or otherwise ensure that no data race will happen.
> + #[inline]
> + pub unsafe fn f_pos(&self) -> i64 {
> + unsafe { (*self.as_ptr()).f_pos }
> + }
> +
> + /// Set the file position.
> + ///
> + /// # Safety
> + ///
> + /// You must hold the fpos lock or otherwise ensure that no data race will happen.
> + #[inline]
> + pub unsafe fn set_f_pos(&self, value: i64) {
> + unsafe { (*self.as_ptr()).f_pos = value };
> + }

I don't think we want to expose f_pos with its weird locking rule
through rust wrappers. Ideally, it's completely opaque to the callers
and not accessed directly at all.