[patch] mtd: use correct error codes in debugfs_create()

[patch] mtd: use correct error codes in debugfs_create()

[Dan Carpenter]

The test here is reversed.  It should be that if "dent" is a valid error
code then we use it, otherwise if it is NULL then use -ENODEV.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index 63cb1d7..3916782 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -236,7 +236,7 @@ int ubi_debugfs_init(void)
 
 	dfs_rootdir = debugfs_create_dir("ubi", NULL);
 	if (IS_ERR_OR_NULL(dfs_rootdir)) {
-		int err = dfs_rootdir ? -ENODEV : PTR_ERR(dfs_rootdir);
+		int err = dfs_rootdir ? PTR_ERR(dfs_rootdir) : -ENODEV;
 
 		ubi_err("cannot create \"ubi\" debugfs directory, error %d\n",
 			err);
diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index cb38f3d..f06863a 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -530,7 +530,7 @@ static int nandsim_debugfs_create(struct nandsim *dev)
 
 	dent = debugfs_create_dir("nandsim", NULL);
 	if (IS_ERR_OR_NULL(dent)) {
-		int err = dent ? -ENODEV : PTR_ERR(dent);
+		int err = dent ? PTR_ERR(dent) : -ENODEV;
 
 		NS_ERR("cannot create \"nandsim\" debugfs directory, err %d\n",
 			err);

[patch] mtd: use correct error codes in debugfs_create()

[Dan Carpenter]

The test here is reversed.  It should be that if "dent" is a valid error
code then we use it, otherwise if it is NULL then use -ENODEV.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index 63cb1d7..3916782 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -236,7 +236,7 @@ int ubi_debugfs_init(void)
 
 	dfs_rootdir = debugfs_create_dir("ubi", NULL);
 	if (IS_ERR_OR_NULL(dfs_rootdir)) {
-		int err = dfs_rootdir ? -ENODEV : PTR_ERR(dfs_rootdir);
+		int err = dfs_rootdir ? PTR_ERR(dfs_rootdir) : -ENODEV;
 
 		ubi_err("cannot create \"ubi\" debugfs directory, error %d\n",
 			err);
diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index cb38f3d..f06863a 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -530,7 +530,7 @@ static int nandsim_debugfs_create(struct nandsim *dev)
 
 	dent = debugfs_create_dir("nandsim", NULL);
 	if (IS_ERR_OR_NULL(dent)) {
-		int err = dent ? -ENODEV : PTR_ERR(dent);
+		int err = dent ? PTR_ERR(dent) : -ENODEV;
 
 		NS_ERR("cannot create \"nandsim\" debugfs directory, err %d\n",
 			err);

Re: [patch] mtd: use correct error codes in debugfs_create()

[Dan Carpenter]

On Fri, Jul 19, 2013 at 08:49:38AM +0300, Dan Carpenter wrote:
> The test here is reversed.  It should be that if "dent" is a valid error
> code then we use it, otherwise if it is NULL then use -ENODEV.
> 

People have explained the debugfs_create API to me better and now
I'm not sure what to do here...  debugfs_create_dir() returns a
NULL on error and if debugfs is not enabled then it returns -ENODEV.
We test for if (!IS_ENABLED(CONFIG_DEBUG_FS)) earlier in the
function so it can only return NULL.

Probably I should just change the test to only test for NULL?

Otherwise I think my patch is correct.  The original code will
return success on error (NULL return) and that was not intended.

regards,
dan carpenter

Re: [patch] mtd: use correct error codes in debugfs_create()

[Dan Carpenter]

On Fri, Jul 19, 2013 at 08:49:38AM +0300, Dan Carpenter wrote:
> The test here is reversed.  It should be that if "dent" is a valid error
> code then we use it, otherwise if it is NULL then use -ENODEV.
> 

People have explained the debugfs_create API to me better and now
I'm not sure what to do here...  debugfs_create_dir() returns a
NULL on error and if debugfs is not enabled then it returns -ENODEV.
We test for if (!IS_ENABLED(CONFIG_DEBUG_FS)) earlier in the
function so it can only return NULL.

Probably I should just change the test to only test for NULL?

Otherwise I think my patch is correct.  The original code will
return success on error (NULL return) and that was not intended.

regards,
dan carpenter

Re: [patch] mtd: use correct error codes in debugfs_create()

[walter harms]

Am 22.07.2013 07:56, schrieb Dan Carpenter:
> On Fri, Jul 19, 2013 at 08:49:38AM +0300, Dan Carpenter wrote:
>> The test here is reversed.  It should be that if "dent" is a valid error
>> code then we use it, otherwise if it is NULL then use -ENODEV.
>>
> 
> People have explained the debugfs_create API to me better and now
> I'm not sure what to do here...  debugfs_create_dir() returns a
> NULL on error and if debugfs is not enabled then it returns -ENODEV.
> We test for if (!IS_ENABLED(CONFIG_DEBUG_FS)) earlier in the
> function so it can only return NULL.
> 
> Probably I should just change the test to only test for NULL?
> 
> Otherwise I think my patch is correct.  The original code will
> return success on error (NULL return) and that was not intended.
> 

Hi Dan,
i have a simple question, what to do in case of error ?

Situation 1: debugfs_create_dir returns NULL
something went wrong dir not created
if all other debugfs_ function will check for NULL someone could even ignore the check
and let the others handle that.

Situation 2: debugfs_create_dir returns ENODOV
The man page says: please ignore
I only use i could come up with is to inform the user that no debugfs is available.
But the returning pointer would now point to an error an not the expected dentry.
So have to you make sure that it becomes NULL otherwise the other debugfs function will go crasy.
(Can this happen actually ?)
Did i miss something ?

In Summa:
may it is better for debugfs_ to return NULL instead of an error ?

re,
 wh

Re: [patch] mtd: use correct error codes in debugfs_create()

[walter harms]

Am 22.07.2013 07:56, schrieb Dan Carpenter:
> On Fri, Jul 19, 2013 at 08:49:38AM +0300, Dan Carpenter wrote:
>> The test here is reversed.  It should be that if "dent" is a valid error
>> code then we use it, otherwise if it is NULL then use -ENODEV.
>>
> 
> People have explained the debugfs_create API to me better and now
> I'm not sure what to do here...  debugfs_create_dir() returns a
> NULL on error and if debugfs is not enabled then it returns -ENODEV.
> We test for if (!IS_ENABLED(CONFIG_DEBUG_FS)) earlier in the
> function so it can only return NULL.
> 
> Probably I should just change the test to only test for NULL?
> 
> Otherwise I think my patch is correct.  The original code will
> return success on error (NULL return) and that was not intended.
> 

Hi Dan,
i have a simple question, what to do in case of error ?

Situation 1: debugfs_create_dir returns NULL
something went wrong dir not created
if all other debugfs_ function will check for NULL someone could even ignore the check
and let the others handle that.

Situation 2: debugfs_create_dir returns ENODOV
The man page says: please ignore
I only use i could come up with is to inform the user that no debugfs is available.
But the returning pointer would now point to an error an not the expected dentry.
So have to you make sure that it becomes NULL otherwise the other debugfs function will go crasy.
(Can this happen actually ?)
Did i miss something ?

In Summa:
may it is better for debugfs_ to return NULL instead of an error ?

re,
 wh

Re: [patch] mtd: use correct error codes in debugfs_create()

[Dan Carpenter]

The debugfs API is a bit confusing initialy but it's straight
foward to use.  You call:

	dfs_rootdir = debugfs_create_dir(...);

If it returns NULL then you return an error code.  If debugfs is
not enabled then it returns ERR_PTR(-ENODEV) but you don't normally
need to test for it.  After all later when you call:

	debugfs_create_file("wear_report", S_IRUSR,
			    dfs_rootdir, dev, &dfs_fops);

That function is just a no-op because debugfs is disabled.

The problem here is that we test for IS_ERR_OR_NULL() instead of
just if (!dfs_rootdir) which is wrong.  Also if we do hit an error
we return success because the true false bit are reversed.

regards,
dan carpenter
> 

Re: [patch] mtd: use correct error codes in debugfs_create()

[Dan Carpenter]

The debugfs API is a bit confusing initialy but it's straight
foward to use.  You call:

	dfs_rootdir = debugfs_create_dir(...);

If it returns NULL then you return an error code.  If debugfs is
not enabled then it returns ERR_PTR(-ENODEV) but you don't normally
need to test for it.  After all later when you call:

	debugfs_create_file("wear_report", S_IRUSR,
			    dfs_rootdir, dev, &dfs_fops);

That function is just a no-op because debugfs is disabled.

The problem here is that we test for IS_ERR_OR_NULL() instead of
just if (!dfs_rootdir) which is wrong.  Also if we do hit an error
we return success because the true false bit are reversed.

regards,
dan carpenter
> 

[patch v2] mtd: tiny debugfs related fixes

[Dan Carpenter]

debugfs_create_dir() can't return an error pointer because we tested for
IS_ENABLED(CONFIG_DEBUG_FS) earlier so we don't need to test for that.
Also the way debugfs is designed, callers should not normally care about
error pointer returns.

The current code is a little buggy because it return success if
debugfs_create_dir() fails instead of -ENODEV which was intended.

I have also changed the error message slightly.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: completely different

diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index 63cb1d7..d3c99a6 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -235,12 +235,9 @@ int ubi_debugfs_init(void)
 		return 0;
 
 	dfs_rootdir = debugfs_create_dir("ubi", NULL);
-	if (IS_ERR_OR_NULL(dfs_rootdir)) {
-		int err = dfs_rootdir ? -ENODEV : PTR_ERR(dfs_rootdir);
-
-		ubi_err("cannot create \"ubi\" debugfs directory, error %d\n",
-			err);
-		return err;
+	if (!dfs_rootdir) {
+		ubi_err("cannot create \"ubi\" debugfs directory.\n");
+		return -ENODEV;
 	}
 
 	return 0;
diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index cb38f3d..3971465 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -523,24 +523,20 @@ static int nandsim_debugfs_create(struct nandsim *dev)
 {
 	struct nandsim_debug_info *dbg = &dev->dbg;
 	struct dentry *dent;
-	int err;
 
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
 	dent = debugfs_create_dir("nandsim", NULL);
-	if (IS_ERR_OR_NULL(dent)) {
-		int err = dent ? -ENODEV : PTR_ERR(dent);
-
-		NS_ERR("cannot create \"nandsim\" debugfs directory, err %d\n",
-			err);
-		return err;
+	if (!dent) {
+		NS_ERR("cannot create \"nandsim\" debugfs directory.");
+		return -ENODEV;
 	}
 	dbg->dfs_root = dent;
 
 	dent = debugfs_create_file("wear_report", S_IRUSR,
 				   dbg->dfs_root, dev, &dfs_fops);
-	if (IS_ERR_OR_NULL(dent))
+	if (!dent)
 		goto out_remove;
 	dbg->dfs_wear_report = dent;
 
@@ -548,8 +544,7 @@ static int nandsim_debugfs_create(struct nandsim *dev)
 
 out_remove:
 	debugfs_remove_recursive(dbg->dfs_root);
-	err = dent ? PTR_ERR(dent) : -ENODEV;
-	return err;
+	return -ENODEV;
 }
 
 /**

[patch v2] mtd: tiny debugfs related fixes

[Dan Carpenter]

debugfs_create_dir() can't return an error pointer because we tested for
IS_ENABLED(CONFIG_DEBUG_FS) earlier so we don't need to test for that.
Also the way debugfs is designed, callers should not normally care about
error pointer returns.

The current code is a little buggy because it return success if
debugfs_create_dir() fails instead of -ENODEV which was intended.

I have also changed the error message slightly.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: completely different

diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index 63cb1d7..d3c99a6 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -235,12 +235,9 @@ int ubi_debugfs_init(void)
 		return 0;
 
 	dfs_rootdir = debugfs_create_dir("ubi", NULL);
-	if (IS_ERR_OR_NULL(dfs_rootdir)) {
-		int err = dfs_rootdir ? -ENODEV : PTR_ERR(dfs_rootdir);
-
-		ubi_err("cannot create \"ubi\" debugfs directory, error %d\n",
-			err);
-		return err;
+	if (!dfs_rootdir) {
+		ubi_err("cannot create \"ubi\" debugfs directory.\n");
+		return -ENODEV;
 	}
 
 	return 0;
diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index cb38f3d..3971465 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -523,24 +523,20 @@ static int nandsim_debugfs_create(struct nandsim *dev)
 {
 	struct nandsim_debug_info *dbg = &dev->dbg;
 	struct dentry *dent;
-	int err;
 
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
 	dent = debugfs_create_dir("nandsim", NULL);
-	if (IS_ERR_OR_NULL(dent)) {
-		int err = dent ? -ENODEV : PTR_ERR(dent);
-
-		NS_ERR("cannot create \"nandsim\" debugfs directory, err %d\n",
-			err);
-		return err;
+	if (!dent) {
+		NS_ERR("cannot create \"nandsim\" debugfs directory.");
+		return -ENODEV;
 	}
 	dbg->dfs_root = dent;
 
 	dent = debugfs_create_file("wear_report", S_IRUSR,
 				   dbg->dfs_root, dev, &dfs_fops);
-	if (IS_ERR_OR_NULL(dent))
+	if (!dent)
 		goto out_remove;
 	dbg->dfs_wear_report = dent;
 
@@ -548,8 +544,7 @@ static int nandsim_debugfs_create(struct nandsim *dev)
 
 out_remove:
 	debugfs_remove_recursive(dbg->dfs_root);
-	err = dent ? PTR_ERR(dent) : -ENODEV;
-	return err;
+	return -ENODEV;
 }
 
 /**