From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx185.postini.com [74.125.245.185]) by kanga.kvack.org (Postfix) with SMTP id F387F6B0044 for ; Sat, 28 Apr 2012 15:02:27 -0400 (EDT) Received: by dadq36 with SMTP id q36so2492853dad.8 for ; Sat, 28 Apr 2012 12:02:27 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20120426162108.b654a920.akpm@linux-foundation.org> References: <1335383992-19419-1-git-send-email-sasikanth.v19@gmail.com> <20120426162108.b654a920.akpm@linux-foundation.org> Date: Sun, 29 Apr 2012 00:32:26 +0530 Message-ID: Subject: Re: [PATCH 2/2] mm: memblock - Handled failure of debug fs entries creation From: Sasikanth babu Content-Type: multipart/alternative; boundary=047d7b2e4f62a8228604bec1dcdf Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: Tejun Heo , "H. Peter Anvin" , Ingo Molnar , linux-mm@kvack.org, linux-kernel@vger.kernel.org --047d7b2e4f62a8228604bec1dcdf Content-Type: text/plain; charset=ISO-8859-1 On Fri, Apr 27, 2012 at 4:51 AM, Andrew Morton wrote: > On Thu, 26 Apr 2012 01:29:52 +0530 > Sasikantha babu wrote: > > > 1) Removed already created debug fs entries on failure > > > > 2) Fixed coding style 80 char per line > > > > Signed-off-by: Sasikantha babu > > --- > > mm/memblock.c | 14 +++++++++++--- > > 1 files changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > index a44eab3..5553723 100644 > > --- a/mm/memblock.c > > +++ b/mm/memblock.c > > @@ -966,11 +966,19 @@ static int __init memblock_init_debugfs(void) > > { > > struct dentry *root = debugfs_create_dir("memblock", NULL); > > if (!root) > > - return -ENXIO; > > - debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, > &memblock_debug_fops); > > - debugfs_create_file("reserved", S_IRUGO, root, &memblock.reserved, > &memblock_debug_fops); > > + return -ENOMEM; > > hm, why the switch to -ENOMEM? > > Just for consistency (But its dumb mistake I made). > Fact is, debugfs_create_dir() and debugfs_create_file() are stupid > interfaces which don't provide the caller (and hence the user) with any > information about why they failed. Perhaps memblock_init_debugfs() > should return -EWESUCK. > I'm working on a patch which address this issue. debugfs_create_XXX calls will return proper error codes, and fixing the existing code not each and every part but the code which handles the values returned by debufs_create_XXX otherwise it will break the existing functionality . (any suggestions or opinions ?) . Thanks Sasi > > > + if (!debugfs_create_file("memory", S_IRUGO, root, &memblock.memory, > > + &memblock_debug_fops)) > > + goto fail; > > + if (!debugfs_create_file("reserved", S_IRUGO, root, > &memblock.reserved, > > + &memblock_debug_fops)) > > + goto fail; > > > > return 0; > > +fail: > > + debugfs_remove_recursive(root); > > + return -ENOMEM; > > } > > __initcall(memblock_init_debugfs); > > --047d7b2e4f62a8228604bec1dcdf Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Fri, Apr 27, 2012 at 4:5= 1 AM, Andrew Morton <akpm@linux-foundation.org> wrot= e:
On Thu, 26 Apr 2012 01:29:52 +0530
Sasikantha babu <sasikanth.v1= 9@gmail.com> wrote:

> 1) Removed already created debug fs entries on failure
>
> 2) Fixed coding style 80 char per line
>
> Signed-off-by: Sasikantha babu <sasikanth.v19@gmail.com>
> ---
> =A0mm/memblock.c | =A0 14 +++++++++++---
> =A01 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/mm/memblock.c b/mm/memblock.c
> index a44eab3..5553723 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -966,11 +966,19 @@ static int __init memblock_init_debugfs(void) > =A0{
> =A0 =A0 =A0 struct dentry *root =3D debugfs_create_dir("memblock&= quot;, NULL);
> =A0 =A0 =A0 if (!root)
> - =A0 =A0 =A0 =A0 =A0 =A0 return -ENXIO;
> - =A0 =A0 debugfs_create_file("memory", S_IRUGO, root, &= memblock.memory, &memblock_debug_fops);
> - =A0 =A0 debugfs_create_file("reserved", S_IRUGO, root, &am= p;memblock.reserved, &memblock_debug_fops);
> + =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;

hm, why the switch to -ENOMEM?

=A0 =A0 Just for consistency (But its dumb mistake I = made).
=A0
Fact is, debugfs_create_dir() and debugfs_create_file() are stupid
interfaces which don't provide the caller (and hence the user) with any=
information about why they failed. =A0Perhaps memblock_init_debugfs()
should return -EWESUCK.
=A0=A0=A0
=A0=A0 I'm w= orking on a patch which address this issue. debugfs_create_XXX=A0 calls
= =A0=A0 will return proper error codes, and fixing the existing code not eac= h and every part=A0 but the code
=A0=A0 which handles the values returned by debufs_create_XXX otherwise it = will break the existing
=A0=A0 functionality . (any suggestions or opin= ions ?)
=A0=A0 .

Thanks
Sasi

> + =A0 =A0 if (!debugfs_create_file("memory", S_IRUGO, root, = &memblock.memory,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &membloc= k_debug_fops))
> + =A0 =A0 =A0 =A0 =A0 =A0 goto fail;
> + =A0 =A0 if (!debugfs_create_file("reserved", S_IRUGO, root= , &memblock.reserved,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &membloc= k_debug_fops))
> + =A0 =A0 =A0 =A0 =A0 =A0 goto fail;
>
> =A0 =A0 =A0 return 0;
> +fail:
> + =A0 =A0 debugfs_remove_recursive(root);
> + =A0 =A0 return -ENOMEM;
> =A0}
> =A0__initcall(memblock_init_debugfs);


--047d7b2e4f62a8228604bec1dcdf-- -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org