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

[Danilo Krummrich <dakr@kernel.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.

Sounds good to me.

> > > +    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 don't see how picking a (Rust) consitent default is contrary to relying on
recursive remove of directories.

I don't want this API to have "inverse drop semantics"; it's gonna confuse
people and, AFAIK, it'd be the only API doing that.

Your original code with Dir::keep() was perfectly fine and it was still
supporting the "recursive remove case" properly.

So, again this *not* an either-or case, we can have both.