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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, May 05, 2025 at 09:21:51AM -0700, Matthew Maurer wrote:
> On Sat, May 3, 2025 at 5:36 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > 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.
> 
> I'm fine with a new type or a using a more descriptive const generic -
> I did the const-generic to avoid the need to make one variant the
> derefee, which can sometimes complicate structure. I'll default to a
> more descriptive const-generic.
> 
> >
> > > +    /// 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().
> 
> In general for Rust, I agree with you. For this particular case, I
> have a strong disagreement - take a look at calls to
> `debugfs_create_dir` in existing C code - new code chooses to discard
> subdirectory handles when done and rely on the recursive remove of the
> root directory to clean up subdirectories. If you and Greg K-H both
> agree that I should make them drop by default, I'll switch it, but I
> think this is me following the subsystem maintainer's intentions here.

I'm ok with the directory being cleaned up when it goes out of scope.
That makes way more sense overall, and will prevent leaks and other
messes.

For the C api, what I really don't like is when we were returning a
dentry to the caller, as then they had to manage it.  Ideally I didn't
want them to have to manage anything, hence the "lookup_and_remove"
function I added, all that was needed to remember is the name of the
directory, which the driver almost always already knew.

With Rust it's simpler, you can save off a reference and when it goes
away, it gets cleaned up.  You don't have access to the raw dentry,
which takes away my original objection, and if debugfs is not enabled,
there's no additional storage requirements.

So let's keep it simple here, and rely on lifetime rules when we can.

thanks,

greg k-h