All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] netdevsim: check return value of debugfs_create_dir
@ 2017-12-07  1:02 Prashant Bhole
  2017-12-07  2:33 ` Jakub Kicinski
  0 siblings, 1 reply; 6+ messages in thread
From: Prashant Bhole @ 2017-12-07  1:02 UTC (permalink / raw)
  To: David S . Miller; +Cc: Prashant Bhole, netdev, Jakub Kicinski

- 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 <bhole_prashant_q7@lab.ntt.co.jp>
---
 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next] netdevsim: check return value of debugfs_create_dir
  2017-12-07  1:02 [PATCH net-next] netdevsim: check return value of debugfs_create_dir Prashant Bhole
@ 2017-12-07  2:33 ` Jakub Kicinski
  2017-12-07  4:10   ` Prashant Bhole
  0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2017-12-07  2:33 UTC (permalink / raw)
  To: Prashant Bhole; +Cc: David S . Miller, netdev

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 <bhole_prashant_q7@lab.ntt.co.jp>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH net-next] netdevsim: check return value of debugfs_create_dir
  2017-12-07  2:33 ` Jakub Kicinski
@ 2017-12-07  4:10   ` Prashant Bhole
  2017-12-07  6:05     ` Jakub Kicinski
  0 siblings, 1 reply; 6+ messages in thread
From: Prashant Bhole @ 2017-12-07  4:10 UTC (permalink / raw)
  To: 'Jakub Kicinski'; +Cc: 'David S . Miller', netdev


> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> 
> 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 <bhole_prashant_q7@lab.ntt.co.jp>
> 
> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next] netdevsim: check return value of debugfs_create_dir
  2017-12-07  4:10   ` Prashant Bhole
@ 2017-12-07  6:05     ` Jakub Kicinski
  2017-12-07  8:56       ` Prashant Bhole
  0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2017-12-07  6:05 UTC (permalink / raw)
  To: Prashant Bhole; +Cc: 'David S . Miller', netdev

On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:
> > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > 
> > 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 <bhole_prashant_q7@lab.ntt.co.jp>  
> > 
> > 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH net-next] netdevsim: check return value of debugfs_create_dir
  2017-12-07  6:05     ` Jakub Kicinski
@ 2017-12-07  8:56       ` Prashant Bhole
  2017-12-07 21:20         ` Jakub Kicinski
  0 siblings, 1 reply; 6+ messages in thread
From: Prashant Bhole @ 2017-12-07  8:56 UTC (permalink / raw)
  To: 'Jakub Kicinski'; +Cc: 'David S . Miller', netdev



> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> 
> On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:
> > > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > >
> > > 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 <bhole_prashant_q7@lab.ntt.co.jp>
> > >
> > > 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?

-Prashant

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next] netdevsim: check return value of debugfs_create_dir
  2017-12-07  8:56       ` Prashant Bhole
@ 2017-12-07 21:20         ` Jakub Kicinski
  0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2017-12-07 21:20 UTC (permalink / raw)
  To: Prashant Bhole; +Cc: 'David S . Miller', netdev

On Thu, 7 Dec 2017 17:56:37 +0900, Prashant Bhole wrote:
> > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > 
> > On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:  
> > > > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > > >
> > > > 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 <bhole_prashant_q7@lab.ntt.co.jp>  
> > > >
> > > > 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-07 21:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-07  1:02 [PATCH net-next] netdevsim: check return value of debugfs_create_dir Prashant Bhole
2017-12-07  2:33 ` Jakub Kicinski
2017-12-07  4:10   ` Prashant Bhole
2017-12-07  6:05     ` Jakub Kicinski
2017-12-07  8:56       ` Prashant Bhole
2017-12-07 21:20         ` Jakub Kicinski

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.