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

[Danilo Krummrich <dakr@kernel.org>]

On Fri, May 02, 2025 at 07:49:30PM +0000, Matthew Maurer wrote:
> +/// Owning handle to a DebugFS directory.
> +///
> +/// This directory will be cleaned up when it goes out of scope.
> +///
> +/// # Invariants
> +///
> +/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
> +#[repr(transparent)]
> +pub struct Dir<'a, const KEEP: bool = false> {

Why did you move to a const generic, rather than a new type? What's the
advantage? AFAICS, it just makes it less obvious to see from the type itself how
it will behave. Reading Dir<true> doesn't make it obvious what it does.

While I prefer a new type over the const generic, I'm fine with it. But I think
we should use something more descriptive than a bool. Please see
device::DeviceContext for reference.

> +    /// Create a DebugFS subdirectory.
> +    ///
> +    /// This returns a [`Dir<'_, true>`], which will not be automatically cleaned up when it
> +    /// leaves scope.
> +    /// To convert this to a handle governing the lifetime of the directory, use [`Dir::owning`].
> +    ///
> +    /// Regardless of conversion, 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, true> {
> +        Dir::create(name, Some(self))
> +    }

The default should be that the directory is removed when the Dir instance is
dropped.

The common case (which people expect) is that an object is cleaned up on drop().

> +impl<'a> Dir<'a, true> {
> +    /// Upgrade a non-owning directory to one which will be removed on drop.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let debugfs = Dir::new(c_str!("parent"));
> +    /// let subdir = debugfs.subdir(c_str!("child"));
> +    /// // If subdir were dropped, the directory would not be removed.
> +    /// let owned_subdir = subdir.owning();
> +    /// // If owned_subdir is dropped, "child" will be removed.
> +    /// ```
> +    pub fn owning(self) -> Dir<'a, false> {
> +        Dir {
> +            dir: self.dir,
> +            _phantom: self._phantom,
> +        }
> +    }

As mentioned above, please make it the other way around.