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

[Andrew Morton]

On Sat, 15 Aug 2015 09:07:30 -0700 Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> > > 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.

wha?  We have thousands and thousands of assertions in the kernel and
there's nothing the user can do about any them, apart from sending us a
bug report.

If debugfs_create_file() fails then something is messed up in the
kernel.  The kernel error shouldn't just be ignored!  It should be
reported and fixed.

> > 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...)

Yeah.  That's just wrong.  debugfs is just kernel code.  If it goes
wrong we should handle that in the usual way, so it gets fixed.
--
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/