Re: [PATCH v5 1/4] rust: debugfs: Bind DebugFS directory creation

["Benno Lossin" <lossin@kernel.org>]

On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> +impl<'a> Dir<'a> {
> +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> +    #[cfg(CONFIG_DEBUG_FS)]
> +    fn create<'b>(name: &CStr, parent: Option<&'a Dir<'b>>) -> Self {
> +        let parent_ptr = match parent {
> +            Some(parent) => parent.0.as_ptr(),
> +            None => core::ptr::null_mut(),
> +        };
> +        // SAFETY:
> +        // * `name` argument points to a NUL-terminated string that lives across the call, by
> +        //   invariants of `&CStr`.
> +        // * If `parent` is `None`, `parent` accepts null pointers to mean create at root.
> +        // * If `parent` is `Some`, `parent` accepts live dentry debugfs pointers.
> +        //   so we can call `Self::from_ptr`.
> +        let dir = unsafe { bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr) };
> +
> +        // SAFETY: `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
> +        Self(unsafe { Entry::from_ptr(dir) })
> +    }
> +
> +    #[cfg(not(CONFIG_DEBUG_FS))]
> +    fn create<'b>(_name: &CStr, _parent: Option<&'a Dir<'b>>) -> Self {
> +        Self(Entry::new())
> +    }
> +
> +    /// Create a DebugFS subdirectory.

I'm not familiar with debugfs, if I run `Dir::create(c"foo", None)`
twice, will both of the returned values refer to the same or different
directories? What if I give a parent?
If the answer in both cases is that they will refer to the same
directory, then I'd change the docs to mention that. So instead of
"Creates" we could say "Finds or creates" or something better.
If they refer to different files, then I am confused how that would look
like in user-land :)

> +    ///
> +    /// Subdirectory handles cannot outlive the directory handle they were created from.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let parent = Dir::new(c_str!("parent"));
> +    /// let child = parent.subdir(c_str!("child"));
> +    /// ```
> +    pub fn subdir<'b>(&'b self, name: &CStr) -> Dir<'b> {
> +        Dir::create(name, Some(self))
> +    }
> +
> +    /// Create a new directory in DebugFS at the root.

Ditto here.

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let debugfs = Dir::new(c_str!("parent"));
> +    /// ```
> +    pub fn new(name: &CStr) -> Self {
> +        Dir::create(name, None)
> +    }

I think it would make more sense for this function to return
`Dir<'static>`.

---
Cheers,
Benno

> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index c3762e80b314316b4b0cee3bfd9442f8f0510b91..86f6055b828d5f711578293d8916a517f2436977 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -45,6 +45,7 @@
>  #[doc(hidden)]
>  pub mod build_assert;
>  pub mod cred;
> +pub mod debugfs;
>  pub mod device;
>  pub mod device_id;
>  pub mod devres;