Message-Id: <D9VQ8VDGD557.2KJ4SKUVEQGDQ@kernel.org>
Date: Wed, 14 May 2025 10:06:18 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Matthew Maurer" <mmaurer@...gle.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...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@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 "Sami Tolvanen" <samitolvanen@...gle.com>, "Timur Tabi" <ttabi@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v5 2/4] rust: debugfs: Bind file creation for long-lived
 Display

On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index ed1aba6d700d064dbfd7e923dbcbf80b9acf5361..4a138717bd0fdb320033d07446a192c9f520a17b 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -46,6 +47,19 @@ unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
>          }
>      }
>  
> +    /// Constructs a new DebugFS [`Entry`] from the underlying pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
> +    /// live DebugFS directory.
> +    #[cfg(not(CONFIG_DEBUG_FS))]
> +    unsafe fn from_ptr(_entry: *mut bindings::dentry) -> Self {
> +        Self {

Why duplicate this function and not just do this to the existing
function?:

    unsafe fn from_ptr(entry: *mut bindings::dentry) -> Self {
        #[cfg(not(CONFIG_DEBUG_FS))]
        let _ = entry;
        Self {
            #[cfg(CONFIG_DEBUG_FS)]
            entry,
            _phantom: PhantomData,
        }
    }

> +            _phantom: PhantomData,
> +        }
> +    }
> +
>      #[cfg(not(CONFIG_DEBUG_FS))]
>      fn new() -> Self {
>          Self {
> @@ -124,6 +138,57 @@ pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
>          Dir::create(name, Some(self))
>      }
>  
> +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> +    /// [`Display::fmt`] on the provided reference.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
> +    /// dir.display_file(c_str!("foo"), &200);
> +    /// // "my_debugfs_dir/foo" now contains the number 200.
> +    /// ```
> +    pub fn display_file<'b, T: Display + Sized>(
> +        &'b self,
> +        name: &CStr,
> +        data: &'static T,
> +    ) -> File<'b> {
> +        // SAFETY:
> +        // * `name` is a NUL-terminated C string, living across the call, by `CStr` invariant.
> +        // * `parent` is a live `dentry` since we have a reference to it.
> +        // * `vtable` is all stock `seq_file` implementations except for `open`.
> +        //   `open`'s only requirement beyond what is provided to all open functions is that the
> +        //   inode's data pointer must point to a `T` that will outlive it, which we know because
> +        //   we have a static reference.
> +        #[cfg(CONFIG_DEBUG_FS)]
> +        let ptr = unsafe {
> +            bindings::debugfs_create_file_full(
> +                name.as_char_ptr(),
> +                0o444,
> +                self.0.as_ptr(),
> +                data as *const _ as *mut _,
> +                core::ptr::null(),
> +                &<T as DisplayFile>::VTABLE,
> +            )
> +        };
> +
> +        #[cfg(not(CONFIG_DEBUG_FS))]
> +        let ptr = {
> +            // Mark parameters used
> +            let (_, _) = (name, data);

`let _ = (name, data);` should be sufficient.

> +            crate::error::code::ENODEV.to_ptr()
> +        };
> +
> +        // SAFETY: `debugfs_create_file_full` either returns an error code or a legal
> +        // dentry pointer, and without `CONFIG_DEBUGFS` we return an error pointer, so
> +        // `Entry::from_ptr` is safe to call here.
> +        let entry = unsafe { Entry::from_ptr(ptr) };
> +
> +        File(entry)
> +    }
> +
>      /// Create a new directory in DebugFS at the root.
>      ///
>      /// # Examples
> @@ -137,3 +202,70 @@ pub fn new(name: &CStr) -> Self {
>          Dir::create(name, None)
>      }
>  }
> +/// Handle to a DebugFS file.
> +#[repr(transparent)]
> +pub struct File<'a>(Entry<'a>);
> +
> +#[cfg(CONFIG_DEBUG_FS)]
> +mod helpers {
> +    use crate::seq_file::SeqFile;
> +    use crate::seq_print;
> +    use core::fmt::Display;
> +
> +    /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * `inode`'s private pointer must point to a value of type `T` which will outlive the `inode`
> +    ///   and will not be mutated during this call.
> +    /// * `file` must point to a live, not-yet-initialized file object.
> +    pub(crate) unsafe extern "C" fn display_open<T: Display>(

Why do these functions need to be pub?

---
Cheers,
Benno

> +        inode: *mut bindings::inode,
> +        file: *mut bindings::file,
> +    ) -> i32 {
> +        // SAFETY:
> +        // * `file` is acceptable by caller precondition.
> +        // * `print_act` will be called on a `seq_file` with private data set to the third argument,
> +        //   so we meet its safety requirements.
> +        // * The `data` pointer passed in the third argument is a valid `T` pointer that outlives
> +        //   this call by caller preconditions.
> +        unsafe { bindings::single_open(file, Some(display_act::<T>), (*inode).i_private) }
> +    }

Powered by blists - more mailing lists