Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Geert Uytterhoeven]

Hi Greg,

On Mon, May 24, 2021 at 12:18 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, May 24, 2021 at 11:51:42AM +0200, Geert Uytterhoeven wrote:
> > On Mon, May 24, 2021 at 11:41 AM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> > > > On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > No one checks the return value of debugfs_create_bool(), as it's not
> > > > > needed, so make the return value void, so that no one tries to do so in
> > > >
> > > > Please explain in the patch description why it is not needed.
> > >
> > > Because you just do not need it, like almost all other debugfs calls
> > > now.
> >
> > Why do I just not need it?
>
> Let me flip it around, why do you need it?  There are no in-kernel users
> of the return value anymore so what code requires this pointer now?

There still are a few users of other members in the family, and some
of them are meant to be removed without removing the full parent
directory.  Having all debugfs_create_*() functions behave the same
is a bonus.

> The goal of removing these dentry pointers was that users were somehow
> using the return value to determine code paths (like erroring out of
> files were not created).  Debugfs code working or not working should
> never matter, this is only for debugging features and we had a number of
> cases where if debugfs was acting up, other "real" things would stop
> working.
>
> Yes, there are a few exceptions that some of the perf/trace people point
> out, and they still check the return value of creating individual
> debugfs files for good reasons.  But for any driver or a "normal" kernel
> subsystem, they should not be doing that as it's wasteful and pointless.
>
> debugfs is supposed to be "simple" and almost "fire and forget" as
> possible.  By removing the ability to check return values, it helps
> achieve this as I have seen all sorts of errors when trying to check the
> return values of debugfs calls, mostly where people were thinking they
> were checking for an error, yet they really were not.
>
> So for the past few years, I've been slowly cleaning this all up,
> removing the ability to get using the debugfs api wrong, which is the
> end-goal here.  By allowing a return value to be forced to be checked,
> developers have the ability to get it wrong (and they did.)

"to be forced to be checked" applies to _must_check only.

> > > If you really do need the file dentry, there is still a call to create
> > > it, and you can always query debugfs for the dentry after it is created
> >
> > ... and will have to duplicate debugfs_create_bool() and friends, but
> > with a return value.  This may introduce bugs, and will complicate
> > maintenance, as any change to debugfs_create_bool() means all those
> > copies need to be found and updated, too.
>
> There are no in-kernel users that need to check this return value, so
> what code are we talking about here?

Yeah, you removed the last user in commit 1be4ec2456a7d110 ("scsi:
snic: debugfs: remove local storage of debugfs files") ;-)
As I said, there are a few more for other similar functions.

But if other people are fine with having to call
debugfs_remove(debugfs_lookup(...)), well, let it be like that...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds