Message-ID: <2025043022-travesty-slicing-2089@gregkh>
Date: Wed, 30 Apr 2025 17:23:43 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Matthew Maurer <mmaurer@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Sami Tolvanen <samitolvanen@...gle.com>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

On Wed, Apr 30, 2025 at 08:10:44AM -0700, Matthew Maurer wrote:
> On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> wrote:
> >
> > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> > > is delayed until the more full-featured API, because we need to avoid
> > > the user having an reference to a dir that is recursively removed.
> > >
> > > Signed-off-by: Matthew Maurer <mmaurer@...gle.com>
> >
> > First off, many thanks for doing this.  I like this in general, but I
> > have loads of specific questions/comments.  Don't take that as a
> > criticism of this feature, I really want these bindings to be in the
> > tree and work hopefully better/cleaner than the userspace ones do.
> >
> > First off, the main "rule" of debugfs is that you should NEVER care
> > about the return value of any debugfs function.  So much so that the C
> > side hides errors almost entirely where possible.  I'd like to see this
> > carried through to the Rust side as well, but I think you didn't do that
> > for various "traditional" reasons.
> 
> Sure, I mostly had to do error checking because I was using an
> `ARef<Dir>` to represent a directory, which meant that the underlying
> directory needed to be a valid pointer. Given that you've said that
> the returned `dentry *` should never be used as an actual `dentry`,
> only as an abstract handle to a DebugFS object, that requirement goes
> away, and I can remove the error handling code and always successfully
> return a `Dir`, even in cases where an error has occurred.

Great!

Except when debugfs is not enabled, then what are you going to return?
The same structure, or an error?

I'd vote for the same pointer to the structure, just to make it more
obvious that this is a totally opaque thing and that no caller should
ever know or care if debugfs is working or even present in the system.

Note that some drivers will want to save a bit of space if debugfs is
not enabled in the build, so be prepared to make the binding work
somehow that way too.  Can you have an "empty" object that takes no
memory?  Or is this too overthinking things?

> > Wait, what?  Why would an explicit drop(parent) be required here?  That
> > feels wrong, and way too complex.  Why can't you rely on the child
> > creation to properly manage this if needed (hint, it shouldn't be.)
> 
> The explicit `drop` is not required for normal usage, it was intended
> to be illustrative - I was trying to explain what the semantics would
> be if the parent `dentry` was released before the child was. As
> before, now that the child will not be an `ARef<Dir>`, and so not
> assumed to be a valid `dentry` pointer, this example won't be relevant
> anymore.

Great!

thanks, hopefully this should make things simpler.

greg k-h

Powered by blists - more mailing lists