All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Michal Hocko <mhocko@kernel.org>
Cc: linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	David Rientjes <rientjes@google.com>,
	Laura Abbott <labbott@redhat.com>,
	linux-mm@kvack.org
Subject: Re: [PATCH] mm: no need to check return value of debugfs_create functions
Date: Tue, 22 Jan 2019 16:52:55 +0100	[thread overview]
Message-ID: <20190122155255.GA20142@kroah.com> (raw)
In-Reply-To: <20190122153102.GJ4087@dhcp22.suse.cz>

On Tue, Jan 22, 2019 at 04:31:02PM +0100, Michal Hocko wrote:
> On Tue 22-01-19 16:21:13, Greg KH wrote:
> [...]
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 022d4cbb3618..18ee657fb918 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -1998,8 +1998,7 @@ DEFINE_SHOW_ATTRIBUTE(memblock_debug);
> >  static int __init memblock_init_debugfs(void)
> >  {
> >  	struct dentry *root = debugfs_create_dir("memblock", NULL);
> > -	if (!root)
> > -		return -ENXIO;
> > +
> >  	debugfs_create_file("memory", 0444, root,
> >  			    &memblock.memory, &memblock_debug_fops);
> >  	debugfs_create_file("reserved", 0444, root,
> 
> I haven't really read the whole patch but this has just hit my eyes. Is
> this a correct behavior?
> 
> Documentations says:
>  * @parent: a pointer to the parent dentry for this file.  This should be a
>  *          directory dentry if set.  If this parameter is NULL, then the
>  *          file will be created in the root of the debugfs filesystem.
> 
> so in case of failure we would get those debugfs files outside of their
> intended scope. I believe it is much more correct to simply not create
> anything, no?

If debugfs_create_dir() returns NULL, then something is really wrong
(you passed it an invalid pointer as the parent dentry, or free memory
is gone), so there's nothing you can do except keep moving forward and
take that result and pass it as any parent pointer you want to.  Your
code logic should never care if a debugfs file is created or not, it is
"fire and forget".

And any result of a debugfs call, like this one, that is to be passed
into another debugfs call, will work just fine if the first one failed
(the second one usually will also fail, which is fine.)

Also, and this is the biggest problem, everyone gets the return value
check wrong thinking NULL will be an error, it is one type of error, but
other ones are also returned and no one checks them properly.  So just
don't check at all, that is the design goal here.

hope this helps,

greg k-h

  reply	other threads:[~2019-01-22 15:53 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-22 15:21 Greg Kroah-Hartman
2019-01-22 15:31 ` Michal Hocko
2019-01-22 15:52   ` Greg Kroah-Hartman [this message]
2019-01-22 16:07     ` Michal Hocko
2019-01-22 16:27       ` Greg Kroah-Hartman
2019-01-22 17:29         ` Michal Hocko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190122155255.GA20142@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=akpm@linux-foundation.org \
    --cc=labbott@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=rientjes@google.com \
    --cc=vbabka@suse.cz \
    --subject='Re: [PATCH] mm: no need to check return value of debugfs_create functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.