Re: [PATCH] mm/memblock: validate the creation of debugfs files

[Greg Kroah-Hartman]

On Sat, Aug 15, 2015 at 12:38:30AM -0700, Andrew Morton wrote:
> On Sat, 15 Aug 2015 13:26:36 +0600 Alexander Kuleshov <kuleshovmail@xxxxxxxxx> wrote:
> 
> > Hello Andrew,
> > 
> > On 08-14-15, Andrew Morton wrote:
> > > On Sat, 15 Aug 2015 01:03:31 +0600 Alexander Kuleshov <kuleshovmail@xxxxxxxxx> wrote:
> > > 
> > > > Signed-off-by: Alexander Kuleshov <kuleshovmail@xxxxxxxxx>
> > > 
> > > There's no changelog.
> > 
> > Yes, will add it if there will be sense in the patch.
> > 
> > > 
> > > Why?  Ignoring the debugfs API return values is standard practice.
> > > 
> > 
> > Yes, but I saw many places where this practice is applicable (for example
> > in the kernel/kprobes and etc.), besides this, the memblock API is used
> > mostly at early stage, so we will have some output if something going wrong.
> 
> The debugfs error-handling rules are something Greg cooked up after one
> too many beers.  I've never understood them, but maybe I continue to
> miss the point.

The "point" is that it should be easy to use, and you don't care if the
file fails to be created because your normal code flow / functionality
does not care if a debugfs file fails to be created.

The only way a debugfs file will fail to be created is if you name
something the same as a file is present, or you passed in the wrong
options, or if you are out of memory, and in all of those cases, there's
nothing a user can do about it.  Yes, when writing your code the first
time, check the error if you want to figure out your logic, but after
that, you don't care.

If debugfs is not enabled, yes, an error will be returned, but you don't
have to care about that, because again, you don't care, and your main
code path is just fine.

So just ignore the return value of debugfs functions, except to save off
pointers that you need to pass back in them later.

> Yes, I agree that if memblock's debugfs_create_file() fails, we want to
> know about it because something needs fixing.

What can be fixed?  Out of memory?  Identical file name?  Nothing a user
can do about that.

> But that's true of
> all(?) debugfs_create_file callsites, so it's a bit silly to add
> warnings to them all.  Why not put the warning into
> debugfs_create_file() itself?  And add a debugfs_create_file_no_warn()
> if there are callsites which have reason to go it alone.  Or add a
> debugfs_create_file_warn() wrapper.

No, it's really not worth it.  The goal of debugfs was to make an api
that is easier to use than procfs which required a bunch of odd return
error checks and you could never tell if the error was due to something
real or if the procfs was not enabled in the kernel.

And it's for debugging files, again, nothing that should be something
you rely on.  If you rely on debugfs files for something, well, you are
using the wrong api (yes, I know all about the trace nightmare...)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/