Re: [PATCH v5 4/4] rust: samples: Add debugfs sample

[Danilo Krummrich <dakr@kernel.org>]

On Wed, May 14, 2025 at 02:55:02PM -0700, Matthew Maurer wrote:
> On Wed, May 14, 2025 at 2:07 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, May 14, 2025 at 09:20:49AM +0200, Benno Lossin wrote:
> > > On Tue May 6, 2025 at 1:51 AM CEST, Matthew Maurer wrote:
> > > > +impl kernel::Module for RustDebugFs {
> > > > +    fn init(_this: &'static ThisModule) -> Result<Self> {
> > > > +        // Create a debugfs directory in the root of the filesystem called "sample_debugfs".
> > > > +        let debugfs = Dir::new(c_str!("sample_debugfs"));
> > > > +
> > > > +        {
> > > > +            // Create a subdirectory, so "sample_debugfs/subdir" now exists.
> > > > +            // We wrap it in `ManuallyDrop` so that the subdirectory is not automatically discarded
> > > > +            // at the end of the scope - it will be cleaned up when `debugfs` is.
> > > > +            let sub = ManuallyDrop::new(debugfs.subdir(c_str!("subdir")));
> > >
> > > I dislike the direct usage of `ManuallyDrop`. To me the usage of
> > > `ManuallyDrop` signifies that one has to opt out of `Drop` without the
> > > support of the wrapped type. But in this case, `Dir` is sometimes
> > > intended to not be dropped, so I'd rather have a `.keep()` function I
> > > saw mentioned somewhere.
> >
> > I agree, if we really want to "officially" support to forget() (sub-)directories
> > and files in order to rely on the recursive cleanup of the "root" directory, it
> > should be covered explicitly by the API. I.e. (sub-)directories and files should
> > have some kind of keep() and / or forget() method, to make it clear that this is
> > supported and by design and won't lead to any leaks.
> 
> OK, I can do this with `.keep()` just being equivalent to
> `ManuallyDrop::new`. Since we now only have "by default, things are
> dropped", rather than two states, we shouldn't need a `.destroy()`
> method.
> 
> >
> > Consequently, this would mean that we'd need something like your proposed const
> > generic on the Dir type, such that keep() / forget() cannot be called on the
> > "root" directory.
> 
> Just to ensure I understand correctly, your opinion is that on
> balance, additional complexity in the API types are worth making it
> less ergonomic to suppress drop on a root directory, keeping in mind
> that they still *can* suppress drop on that directory. You don't want
> the extra API variants to do anything else special other than change
> the ergonomics of `.keep()` (e.g. don't have subdirectories be default
> pre-`.keep()`'d).

Absolutely, yes! Having the keep() method available for "root" directories is
clearly a footgun, because it suggests that it is a valid thing to call for a
"root" directory, but actually is almost always a bug.

What do we have a powerful type system for if we don't take advantage of it to
prevent bugs? :-)