[PATCH net-next] netdevsim: check return value of debugfs_create_dir

- Handled debugfs_create_dir failure in nsim_init()
- Fixed return value of nsim_module_init() when debugfs_create_dir fails

Signed-off-by: Prashant Bhole <redacted>
---
 drivers/net/netdevsim/netdev.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index eb8c679fca9f..6c4e08f46bcb 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c

@@ -147,10 +147,15 @@ struct device_type nsim_dev_type = {
 static int nsim_init(struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
-	int err;
+	int err = -ENOMEM;
 
 	ns->netdev = dev;
 	ns->ddir = debugfs_create_dir(netdev_name(dev), nsim_ddir);
+	if (IS_ERR_OR_NULL(ns->ddir)) {
+		if (ns->ddir)
+			err = PTR_ERR(ns->ddir);
+		goto err;
+	}
 
 	err = nsim_bpf_init(ns);
 	if (err)

@@ -171,6 +176,7 @@ static int nsim_init(struct net_device *dev)
 	nsim_bpf_uninit(ns);
 err_debugfs_destroy:
 	debugfs_remove_recursive(ns->ddir);
+err:
 	return err;
 }
 

@@ -466,11 +472,14 @@ struct dentry *nsim_ddir;
 
 static int __init nsim_module_init(void)
 {
-	int err;
+	int err = -ENOMEM;
 
 	nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
-	if (IS_ERR(nsim_ddir))
-		return PTR_ERR(nsim_ddir);
+	if (IS_ERR_OR_NULL(nsim_ddir)) {
+		if (nsim_ddir)
+			err = PTR_ERR(nsim_ddir);
+		goto err;
+	}
 
 	err = bus_register(&nsim_bus);
 	if (err)

@@ -486,6 +495,7 @@ static int __init nsim_module_init(void)
 	bus_unregister(&nsim_bus);
 err_debugfs_destroy:
 	debugfs_remove_recursive(nsim_ddir);
+err:
 	return err;
 }
 

-- 
2.13.6

On Thu,  7 Dec 2017 10:02:13 +0900, Prashant Bhole wrote:

- Handled debugfs_create_dir failure in nsim_init()
- Fixed return value of nsim_module_init() when debugfs_create_dir fails

Signed-off-by: Prashant Bhole <redacted>

Why?  Failing to expose the state via DebugFS is not fatal to the
driver.

From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]

On Thu,  7 Dec 2017 10:02:13 +0900, Prashant Bhole wrote:

quoted

- Handled debugfs_create_dir failure in nsim_init()
- Fixed return value of nsim_module_init() when debugfs_create_dir
fails

Signed-off-by: Prashant Bhole <redacted>

Why?  Failing to expose the state via DebugFS is not fatal to the driver.

Ok, my intention was to handle the return code properly, which is not needed
as per your comment.
Shall I remove the existing handling in nsim_module_init() in separate
patch?  Because it will prevent netdevsim from loading when debugfs is
disabled.

Thanks,
Prashant

On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:

quoted

From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]

On Thu,  7 Dec 2017 10:02:13 +0900, Prashant Bhole wrote:  

quoted

- Handled debugfs_create_dir failure in nsim_init()
- Fixed return value of nsim_module_init() when debugfs_create_dir
fails

Signed-off-by: Prashant Bhole <redacted>  

Why?  Failing to expose the state via DebugFS is not fatal to the driver.  

Ok, my intention was to handle the return code properly, which is not needed
as per your comment.
Shall I remove the existing handling in nsim_module_init() in separate
patch? 

I was going back and forth on the error handling quite a bit writing
that code.  In the end I decided to leave the module_init check and
check for bpf prog directory.  Former one is mostly useful to make sure
the is no duplicate directory with the same name, the latter to limit
possible false positive in the selftest..

Because it will prevent netdevsim from loading when debugfs is disabled.

Note that netdevsim depends on DEBUG_FS:

config NETDEVSIM
	tristate "Simulated networking device"
	depends on DEBUG_FS

From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]

On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:

quoted

quoted

From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]

On Thu,  7 Dec 2017 10:02:13 +0900, Prashant Bhole wrote:

quoted

- Handled debugfs_create_dir failure in nsim_init()
- Fixed return value of nsim_module_init() when debugfs_create_dir
fails

Signed-off-by: Prashant Bhole <redacted>

Why?  Failing to expose the state via DebugFS is not fatal to the

driver.

quoted

Ok, my intention was to handle the return code properly, which is not
needed as per your comment.
Shall I remove the existing handling in nsim_module_init() in separate
patch?

I was going back and forth on the error handling quite a bit writing that

code.  In

the end I decided to leave the module_init check and check for bpf prog
directory.  Former one is mostly useful to make sure the is no duplicate

directory

with the same name, the latter to limit possible false positive in the

selftest..

Ok. Currently return value is checked with IS_ERR(). But when Debug FS is
enabled, debugfs_create_dir will never return error value. It returns either
NULL or a valid pointer. Shall I replace IS_ERR with NULL check or
IS_ERR_OR_NULL check?

-Prashant

On Thu, 7 Dec 2017 17:56:37 +0900, Prashant Bhole wrote:

quoted

From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]

On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:  

quoted

quoted

From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]

On Thu,  7 Dec 2017 10:02:13 +0900, Prashant Bhole wrote:  

quoted

- Handled debugfs_create_dir failure in nsim_init()
- Fixed return value of nsim_module_init() when debugfs_create_dir
fails

Signed-off-by: Prashant Bhole <redacted>  

Why?  Failing to expose the state via DebugFS is not fatal to the driver.

Ok, my intention was to handle the return code properly, which is not
needed as per your comment.
Shall I remove the existing handling in nsim_module_init() in separate
patch?  

I was going back and forth on the error handling quite a bit writing that code.  In
the end I decided to leave the module_init check and check for bpf prog
directory.  Former one is mostly useful to make sure the is no duplicate directory
with the same name, the latter to limit possible false positive in the selftest..

Ok. Currently return value is checked with IS_ERR(). But when Debug FS is
enabled, debugfs_create_dir will never return error value. It returns either
NULL or a valid pointer. Shall I replace IS_ERR with NULL check or
IS_ERR_OR_NULL check?

Sure.