[PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Phil Carmody]

I knew I invented IS_ERR_OR_NULL for something, and this was probably
it. NULL has lost all information about what the error was, and the most
appropriate error code is ENODEV. However, that's the only error code
that the debugfs functions can return. So basically, any error = ENODEV.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 fs/ubifs/debug.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index dbc093a..7b41b30 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -2578,11 +2578,10 @@ static struct dentry *dfs_rootdir;
 int dbg_debugfs_init(void)
 {
 	dfs_rootdir = debugfs_create_dir("ubifs", NULL);
-	if (IS_ERR(dfs_rootdir)) {
-		int err = PTR_ERR(dfs_rootdir);
+	if (IS_ERR_OR_NULL(dfs_rootdir)) {
 		ubifs_err("cannot create \"ubifs\" debugfs directory, "
-			  "error %d\n", err);
-		return err;
+			  "error %d\n", PTR_ERR(dfs_rootdir));
+		return -ENODEV;
 	}
 
 	return 0;
@@ -2645,47 +2644,44 @@ static const struct file_operations dfs_fops = {
  */
 int dbg_debugfs_init_fs(struct ubifs_info *c)
 {
-	int err;
 	const char *fname;
 	struct dentry *dent;
 	struct ubifs_debug_info *d = c->dbg;
 
 	sprintf(d->dfs_dir_name, "ubi%d_%d", c->vi.ubi_num, c->vi.vol_id);
 	d->dfs_dir = debugfs_create_dir(d->dfs_dir_name, dfs_rootdir);
-	if (IS_ERR(d->dfs_dir)) {
-		err = PTR_ERR(d->dfs_dir);
+	if (IS_ERR_OR_NULL(d->dfs_dir)) {
 		ubifs_err("cannot create \"%s\" debugfs directory, error %d\n",
-			  d->dfs_dir_name, err);
+			  d->dfs_dir_name, PTR_ERR(d->dfs_rootdir));
 		goto out;
 	}
 
 	fname = "dump_lprops";
 	dent = debugfs_create_file(fname, S_IWUGO, d->dfs_dir, c, &dfs_fops);
-	if (IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		goto out_remove;
 	d->dfs_dump_lprops = dent;
 
 	fname = "dump_budg";
 	dent = debugfs_create_file(fname, S_IWUGO, d->dfs_dir, c, &dfs_fops);
-	if (IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		goto out_remove;
 	d->dfs_dump_budg = dent;
 
 	fname = "dump_tnc";
 	dent = debugfs_create_file(fname, S_IWUGO, d->dfs_dir, c, &dfs_fops);
-	if (IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		goto out_remove;
 	d->dfs_dump_tnc = dent;
 
 	return 0;
 
 out_remove:
-	err = PTR_ERR(dent);
 	ubifs_err("cannot create \"%s\" debugfs directory, error %d\n",
-		  fname, err);
+		  fname, PTR_ERR(dent));
 	debugfs_remove_recursive(d->dfs_dir);
 out:
-	return err;
+	return -ENODEV;
 }
 
 /**
-- 
1.7.2.rc1.37.gf8c40

Re: [PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Artem Bityutskiy]

On Wed, 2011-03-23 at 11:51 +0200, Phil Carmody wrote:
> I knew I invented IS_ERR_OR_NULL for something, and this was probably
> it. NULL has lost all information about what the error was, and the most
> appropriate error code is ENODEV. However, that's the only error code
> that the debugfs functions can return. So basically, any error = ENODEV.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

The debugfs code may change and gain more error codes, so I do not think
it is good idea to treat any error as ENODEV. I think only NULL should
be translated to ENODEV instead.

I'll try to re-work your patch accordingly and push it unless you send a
new one.

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Phil Carmody]

On 23/03/11 14:41 +0200, ext Artem Bityutskiy wrote:
> On Wed, 2011-03-23 at 11:51 +0200, Phil Carmody wrote:
> > I knew I invented IS_ERR_OR_NULL for something, and this was probably
> > it. NULL has lost all information about what the error was, and the most
> > appropriate error code is ENODEV. However, that's the only error code
> > that the debugfs functions can return. So basically, any error = ENODEV.
> > 
> > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> The debugfs code may change and gain more error codes, so I do not think
> it is good idea to treat any error as ENODEV. I think only NULL should
> be translated to ENODEV instead.
> 
> I'll try to re-work your patch accordingly and push it unless you send a
> new one.

I had a version like that before I simplified it (and rebased).
I wonder if it's possible to extract it from the garbage in git?

Phil

[PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Phil Carmody]

I knew I invented IS_ERR_OR_NULL for something, and this was probably
it. NULL has lost all information about what the error was, and the most
appropriate error code is ENODEV. However, that's the only error code
that the debugfs functions can return. So basically, any error = ENODEV.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 fs/ubifs/debug.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index dbc093a..7b41b30 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -2578,11 +2578,10 @@ static struct dentry *dfs_rootdir;
 int dbg_debugfs_init(void)
 {
 	dfs_rootdir = debugfs_create_dir("ubifs", NULL);
-	if (IS_ERR(dfs_rootdir)) {
-		int err = PTR_ERR(dfs_rootdir);
+	if (IS_ERR_OR_NULL(dfs_rootdir)) {
 		ubifs_err("cannot create \"ubifs\" debugfs directory, "
-			  "error %d\n", err);
-		return err;
+			  "error %d\n", PTR_ERR(dfs_rootdir));
+		return -ENODEV;
 	}
 
 	return 0;
@@ -2645,47 +2644,44 @@ static const struct file_operations dfs_fops = {
  */
 int dbg_debugfs_init_fs(struct ubifs_info *c)
 {
-	int err;
 	const char *fname;
 	struct dentry *dent;
 	struct ubifs_debug_info *d = c->dbg;
 
 	sprintf(d->dfs_dir_name, "ubi%d_%d", c->vi.ubi_num, c->vi.vol_id);
 	d->dfs_dir = debugfs_create_dir(d->dfs_dir_name, dfs_rootdir);
-	if (IS_ERR(d->dfs_dir)) {
-		err = PTR_ERR(d->dfs_dir);
+	if (IS_ERR_OR_NULL(d->dfs_dir)) {
 		ubifs_err("cannot create \"%s\" debugfs directory, error %d\n",
-			  d->dfs_dir_name, err);
+			  d->dfs_dir_name, PTR_ERR(d->dfs_rootdir));
 		goto out;
 	}
 
 	fname = "dump_lprops";
 	dent = debugfs_create_file(fname, S_IWUGO, d->dfs_dir, c, &dfs_fops);
-	if (IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		goto out_remove;
 	d->dfs_dump_lprops = dent;
 
 	fname = "dump_budg";
 	dent = debugfs_create_file(fname, S_IWUGO, d->dfs_dir, c, &dfs_fops);
-	if (IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		goto out_remove;
 	d->dfs_dump_budg = dent;
 
 	fname = "dump_tnc";
 	dent = debugfs_create_file(fname, S_IWUGO, d->dfs_dir, c, &dfs_fops);
-	if (IS_ERR(dent))
+	if (IS_ERR_OR_NULL(dent))
 		goto out_remove;
 	d->dfs_dump_tnc = dent;
 
 	return 0;
 
 out_remove:
-	err = PTR_ERR(dent);
 	ubifs_err("cannot create \"%s\" debugfs directory, error %d\n",
-		  fname, err);
+		  fname, PTR_ERR(dent));
 	debugfs_remove_recursive(d->dfs_dir);
 out:
-	return err;
+	return -ENODEV;
 }
 
 /**
-- 
1.7.2.rc1.37.gf8c40

Re: [PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Artem Bityutskiy]

On Wed, 2011-03-23 at 15:35 +0200, Phil Carmody wrote:
> I knew I invented IS_ERR_OR_NULL for something, and this was probably
> it. NULL has lost all information about what the error was, and the most
> appropriate error code is ENODEV. However, that's the only error code
> that the debugfs functions can return. So basically, any error = ENODEV.

This is not true that any error is -ENODEV. There are many other errors
possible, e.g., due to an invocation of simple_pin_fs().

I think the right fix would be to fix debugfs and return an error code
in any case.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Phil Carmody]

On 28/03/11 09:51 +0300, ext Artem Bityutskiy wrote:
> On Wed, 2011-03-23 at 15:35 +0200, Phil Carmody wrote:
> > I knew I invented IS_ERR_OR_NULL for something, and this was probably
> > it. NULL has lost all information about what the error was, and the most
> > appropriate error code is ENODEV. However, that's the only error code
> > that the debugfs functions can return. So basically, any error = ENODEV.
> 
> This is not true that any error is -ENODEV. There are many other errors
> possible, e.g., due to an invocation of simple_pin_fs().

I meant that once you've thrown away all information about the actual
error by turning it into NULL, then the broadest brush that covers the
failure modes is ENODEV. That's why I was asked "what's the most generic
'something's gone wrong' code" on the chat channel the other day.
 
> I think the right fix would be to fix debugfs and return an error code
> in any case.

Agree. Alas it might require hitting a lot of both active and dead code.
Is that something you want me to look at? If you do it yourself, please
Cc: me, I'll happily review it.

Phil

Re: [PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Artem Bityutskiy]

On Mon, 2011-03-28 at 13:38 +0300, Phil Carmody wrote:
> > I think the right fix would be to fix debugfs and return an error code
> > in any case.
> 
> Agree. Alas it might require hitting a lot of both active and dead code.
> Is that something you want me to look at? If you do it yourself, please
> Cc: me, I'll happily review it.

No, I do not ask you do change debugfs. Do it only if you fare feeling
enthusiastic about this :-) But I do not feel that way, so won't delve
into the debugfs story :-)

All I mean is that we should not do this:

-       if (IS_ERR(ptr)) {
+       if (IS_ERR_OR_NULL(ptr))
-               int err = PTR_ERR(ptr);
-               return err;
+               return -ENODEV;

But rather

-       if (IS_ERR(ptr)) {
+       if (IS_ERR_OR_NULL(ptr)) {
-               int err = PTR_ERR(ptr);
+               int err = ptr ? PTR_ERR(ptr) : -ENODEV;
                return err;

or something like this.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 1/1] ubifs: debugfs operations may return both ERRs and NULLs

[Artem Bityutskiy]

On Wed, 2011-03-23 at 14:41 +0200, Artem Bityutskiy wrote:
> On Wed, 2011-03-23 at 11:51 +0200, Phil Carmody wrote:
> > I knew I invented IS_ERR_OR_NULL for something, and this was probably
> > it. NULL has lost all information about what the error was, and the most
> > appropriate error code is ENODEV. However, that's the only error code
> > that the debugfs functions can return. So basically, any error = ENODEV.
> > 
> > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> The debugfs code may change and gain more error codes, so I do not think
> it is good idea to treat any error as ENODEV. I think only NULL should
> be translated to ENODEV instead.
> 
> I'll try to re-work your patch accordingly and push it unless you send a
> new one.

Hmm, the other idea would be to fix debugfs code.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)