* [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
@ 2017-12-08 0:52 Prashant Bhole
2017-12-08 1:12 ` Jakub Kicinski
2017-12-08 15:31 ` David Miller
0 siblings, 2 replies; 15+ messages in thread
From: Prashant Bhole @ 2017-12-08 0:52 UTC (permalink / raw)
To: David S . Miller; +Cc: Prashant Bhole, netdev, Jakub Kicinski
Return value is now checked with IS_ERROR_OR_NULL because
debugfs_create_dir doesn't return error value. It either returns
NULL or a valid pointer.
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
drivers/net/netdevsim/netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index eb8c679fca9f..88d8ee2c89da 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
int err;
nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
- if (IS_ERR(nsim_ddir))
+ if (IS_ERR_OR_NULL(nsim_ddir))
return PTR_ERR(nsim_ddir);
err = bus_register(&nsim_bus);
--
2.13.6
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-08 0:52 [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir Prashant Bhole
@ 2017-12-08 1:12 ` Jakub Kicinski
2017-12-08 2:12 ` Prashant Bhole
2017-12-08 15:31 ` David Miller
1 sibling, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2017-12-08 1:12 UTC (permalink / raw)
To: Prashant Bhole; +Cc: David S . Miller, netdev
On Fri, 8 Dec 2017 09:52:50 +0900, Prashant Bhole wrote:
> Return value is now checked with IS_ERROR_OR_NULL because
> debugfs_create_dir doesn't return error value. It either returns
> NULL or a valid pointer.
>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
> drivers/net/netdevsim/netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index eb8c679fca9f..88d8ee2c89da 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
> int err;
>
> nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
> - if (IS_ERR(nsim_ddir))
> + if (IS_ERR_OR_NULL(nsim_ddir))
> return PTR_ERR(nsim_ddir);
PTR_ERR() will not be good in case of NULL, no? But it's anyone's guess
what the correct error code would then be... the EEXIST from
start_creating() I was after is dropped in debugfs_create_dir(), as you
previously pointed out.
Maybe just drop this error handling here and in nsim_bpf_create_prog().
Let's consistently ignore all DebugFS errors. Sorry about going back
and forth on this a little bit.
> err = bus_register(&nsim_bus);
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-08 1:12 ` Jakub Kicinski
@ 2017-12-08 2:12 ` Prashant Bhole
0 siblings, 0 replies; 15+ messages in thread
From: Prashant Bhole @ 2017-12-08 2:12 UTC (permalink / raw)
To: 'Jakub Kicinski'; +Cc: 'David S . Miller', netdev
> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>
> On Fri, 8 Dec 2017 09:52:50 +0900, Prashant Bhole wrote:
> > Return value is now checked with IS_ERROR_OR_NULL because
> > debugfs_create_dir doesn't return error value. It either returns NULL
> > or a valid pointer.
> >
> > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > ---
> > drivers/net/netdevsim/netdev.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/netdevsim/netdev.c
> > b/drivers/net/netdevsim/netdev.c index eb8c679fca9f..88d8ee2c89da
> > 100644
> > --- a/drivers/net/netdevsim/netdev.c
> > +++ b/drivers/net/netdevsim/netdev.c
> > @@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
> > int err;
> >
> > nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
> > - if (IS_ERR(nsim_ddir))
> > + if (IS_ERR_OR_NULL(nsim_ddir))
> > return PTR_ERR(nsim_ddir);
>
> PTR_ERR() will not be good in case of NULL, no? But it's anyone's guess
what
> the correct error code would then be... the EEXIST from
> start_creating() I was after is dropped in debugfs_create_dir(), as you
previously
> pointed out.
>
> Maybe just drop this error handling here and in nsim_bpf_create_prog().
> Let's consistently ignore all DebugFS errors. Sorry about going back and
forth on
> this a little bit.
Ok. I am sending a patch to remove this error handling. Thank you.
-Prashant
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-08 0:52 [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir Prashant Bhole
2017-12-08 1:12 ` Jakub Kicinski
@ 2017-12-08 15:31 ` David Miller
2017-12-11 4:46 ` Prashant Bhole
1 sibling, 1 reply; 15+ messages in thread
From: David Miller @ 2017-12-08 15:31 UTC (permalink / raw)
To: bhole_prashant_q7; +Cc: netdev, jakub.kicinski
From: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Date: Fri, 8 Dec 2017 09:52:50 +0900
> Return value is now checked with IS_ERROR_OR_NULL because
> debugfs_create_dir doesn't return error value. It either returns
> NULL or a valid pointer.
>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
> drivers/net/netdevsim/netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index eb8c679fca9f..88d8ee2c89da 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
> int err;
>
> nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
> - if (IS_ERR(nsim_ddir))
> + if (IS_ERR_OR_NULL(nsim_ddir))
> return PTR_ERR(nsim_ddir);
debugfs_create_dir() should really be fixed, either it uses error pointers
consistently and therefore always provides a suitable error code to return
or it always uses NULL.
This in-between behavior makes using it as an interface painful because
no clear meaning is given to NULL.
So please do the work necessary to make debugfs_create_dir()'s return
semantics clearer and more useful.
Thank you.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-08 15:31 ` David Miller
@ 2017-12-11 4:46 ` Prashant Bhole
2017-12-11 18:40 ` Jakub Kicinski
0 siblings, 1 reply; 15+ messages in thread
From: Prashant Bhole @ 2017-12-11 4:46 UTC (permalink / raw)
To: 'David Miller', jakub.kicinski; +Cc: netdev
> From: David Miller [mailto:davem@davemloft.net]
>
> From: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> Date: Fri, 8 Dec 2017 09:52:50 +0900
>
> > Return value is now checked with IS_ERROR_OR_NULL because
> > debugfs_create_dir doesn't return error value. It either returns NULL
> > or a valid pointer.
> >
> > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > ---
> > drivers/net/netdevsim/netdev.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/netdevsim/netdev.c
> > b/drivers/net/netdevsim/netdev.c index eb8c679fca9f..88d8ee2c89da
> > 100644
> > --- a/drivers/net/netdevsim/netdev.c
> > +++ b/drivers/net/netdevsim/netdev.c
> > @@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
> > int err;
> >
> > nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
> > - if (IS_ERR(nsim_ddir))
> > + if (IS_ERR_OR_NULL(nsim_ddir))
> > return PTR_ERR(nsim_ddir);
>
> debugfs_create_dir() should really be fixed, either it uses error pointers
> consistently and therefore always provides a suitable error code to return
or it
> always uses NULL.
>
> This in-between behavior makes using it as an interface painful because no
clear
> meaning is given to NULL.
>
> So please do the work necessary to make debugfs_create_dir()'s return
> semantics clearer and more useful.
>
> Thank you.
Dave,
Thanks for comments. I will try to fix error handling in netdevsim first.
Jakub,
Let's decide with an example. The typical directory structure for netdevsim
interface is as below:
/sys/kernel/debug/netdevsim/sim0/bpf_bound_progs/
Please let me know if you are ok with following:
1) If debugfs_create_dir() fails in module_init, let's keep it fatal error
with corrected condition:
+ if (IS_ERR_OR_NULL(nsim_ddir))
+ return -ENOMEM;
2) In case sim0 or bpf_bound_progs are fail to create, we need to add
checks before creating any file in them.
-Prashant
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-11 4:46 ` Prashant Bhole
@ 2017-12-11 18:40 ` Jakub Kicinski
2017-12-19 4:45 ` Prashant Bhole
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2017-12-11 18:40 UTC (permalink / raw)
To: Prashant Bhole; +Cc: 'David Miller', netdev
On Mon, 11 Dec 2017 13:46:48 +0900, Prashant Bhole wrote:
> > From: David Miller [mailto:davem@davemloft.net]
> >
> > From: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > Date: Fri, 8 Dec 2017 09:52:50 +0900
> >
> > > Return value is now checked with IS_ERROR_OR_NULL because
> > > debugfs_create_dir doesn't return error value. It either returns NULL
> > > or a valid pointer.
> > >
> > > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > > ---
> > > drivers/net/netdevsim/netdev.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/netdevsim/netdev.c
> > > b/drivers/net/netdevsim/netdev.c index eb8c679fca9f..88d8ee2c89da
> > > 100644
> > > --- a/drivers/net/netdevsim/netdev.c
> > > +++ b/drivers/net/netdevsim/netdev.c
> > > @@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
> > > int err;
> > >
> > > nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
> > > - if (IS_ERR(nsim_ddir))
> > > + if (IS_ERR_OR_NULL(nsim_ddir))
> > > return PTR_ERR(nsim_ddir);
> >
> > debugfs_create_dir() should really be fixed, either it uses error pointers
> > consistently and therefore always provides a suitable error code to return
> or it
> > always uses NULL.
> >
> > This in-between behavior makes using it as an interface painful because no
> clear
> > meaning is given to NULL.
> >
> > So please do the work necessary to make debugfs_create_dir()'s return
> > semantics clearer and more useful.
> >
> > Thank you.
>
> Dave,
> Thanks for comments. I will try to fix error handling in netdevsim first.
>
> Jakub,
> Let's decide with an example. The typical directory structure for netdevsim
> interface is as below:
> /sys/kernel/debug/netdevsim/sim0/bpf_bound_progs/
> Please let me know if you are ok with following:
>
> 1) If debugfs_create_dir() fails in module_init, let's keep it fatal error
> with corrected condition:
> + if (IS_ERR_OR_NULL(nsim_ddir))
> + return -ENOMEM;
>
> 2) In case sim0 or bpf_bound_progs are fail to create, we need to add
> checks before creating any file in them.
Fine with me, although if you fix DebugFS first you could use the real
error from the start here.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-11 18:40 ` Jakub Kicinski
@ 2017-12-19 4:45 ` Prashant Bhole
2017-12-19 14:11 ` David Miller
0 siblings, 1 reply; 15+ messages in thread
From: Prashant Bhole @ 2017-12-19 4:45 UTC (permalink / raw)
To: 'Jakub Kicinski', 'David Miller'; +Cc: netdev
> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>
> On Mon, 11 Dec 2017 13:46:48 +0900, Prashant Bhole wrote:
> > > From: David Miller [mailto:davem@davemloft.net]
> > >
> > > From: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > > Date: Fri, 8 Dec 2017 09:52:50 +0900
> > >
> > > > Return value is now checked with IS_ERROR_OR_NULL because
> > > > debugfs_create_dir doesn't return error value. It either returns
> > > > NULL or a valid pointer.
> > > >
> > > > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > > > ---
> > > > drivers/net/netdevsim/netdev.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/netdevsim/netdev.c
> > > > b/drivers/net/netdevsim/netdev.c index eb8c679fca9f..88d8ee2c89da
> > > > 100644
> > > > --- a/drivers/net/netdevsim/netdev.c
> > > > +++ b/drivers/net/netdevsim/netdev.c
> > > > @@ -469,7 +469,7 @@ static int __init nsim_module_init(void)
> > > > int err;
> > > >
> > > > nsim_ddir = debugfs_create_dir(DRV_NAME, NULL);
> > > > - if (IS_ERR(nsim_ddir))
> > > > + if (IS_ERR_OR_NULL(nsim_ddir))
> > > > return PTR_ERR(nsim_ddir);
> > >
> > > debugfs_create_dir() should really be fixed, either it uses error
> > > pointers consistently and therefore always provides a suitable error
> > > code to return
> > or it
> > > always uses NULL.
> > >
> > > This in-between behavior makes using it as an interface painful
> > > because no
> > clear
> > > meaning is given to NULL.
> > >
> > > So please do the work necessary to make debugfs_create_dir()'s
> > > return semantics clearer and more useful.
> > >
> > > Thank you.
> >
> > Dave,
> > Thanks for comments. I will try to fix error handling in netdevsim
first.
> >
> > Jakub,
> > Let's decide with an example. The typical directory structure for
> > netdevsim interface is as below:
> > /sys/kernel/debug/netdevsim/sim0/bpf_bound_progs/
> > Please let me know if you are ok with following:
> >
> > 1) If debugfs_create_dir() fails in module_init, let's keep it fatal
> > error with corrected condition:
> > + if (IS_ERR_OR_NULL(nsim_ddir))
> > + return -ENOMEM;
> >
> > 2) In case sim0 or bpf_bound_progs are fail to create, we need to add
> > checks before creating any file in them.
>
> Fine with me, although if you fix DebugFS first you could use the real
error from
> the start here.
Jakub, Dave,
Sorry for late reply.
I tried to evaluate whether fixing return value of debugfs_create_dir() (and
friends) will be useful or not because it has not been changed since very
long time. Now I am not much convinced about changing this api.
Important and possible error codes could be -EEXIST and -ENOMEM. Suppose
-EEXIST is returned, IMO the directory shouldn't exists in the first place
because it is specific to particular module. Also, there is no point in
creating file in such directory, because directory owner (creator) might
remove it too. This means there are less chances that api change will be
useful. Please let me know your opinion on it.
If you are ok with above explanation, shall I submit v2 for this patch?
-Prashant
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-19 4:45 ` Prashant Bhole
@ 2017-12-19 14:11 ` David Miller
2017-12-20 0:16 ` Prashant Bhole
0 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2017-12-19 14:11 UTC (permalink / raw)
To: bhole_prashant_q7; +Cc: jakub.kicinski, netdev
From: "Prashant Bhole" <bhole_prashant_q7@lab.ntt.co.jp>
Date: Tue, 19 Dec 2017 13:45:47 +0900
> I tried to evaluate whether fixing return value of debugfs_create_dir() (and
> friends) will be useful or not because it has not been changed since very
> long time. Now I am not much convinced about changing this api.
>
> Important and possible error codes could be -EEXIST and -ENOMEM. Suppose
> -EEXIST is returned, IMO the directory shouldn't exists in the first place
> because it is specific to particular module. Also, there is no point in
> creating file in such directory, because directory owner (creator) might
> remove it too. This means there are less chances that api change will be
> useful. Please let me know your opinion on it.
>
> If you are ok with above explanation, shall I submit v2 for this patch?
Well, something is seriously wrong if the directory exists already.
It could be that two netdevsim modules, independantly compiled, are trying
to be loaded.
Wouldn't it clearly be desirable to fail and not load the module in
that case?
This is why I think ignoring debugfs errors is foolish.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-19 14:11 ` David Miller
@ 2017-12-20 0:16 ` Prashant Bhole
2017-12-20 0:30 ` Jakub Kicinski
0 siblings, 1 reply; 15+ messages in thread
From: Prashant Bhole @ 2017-12-20 0:16 UTC (permalink / raw)
To: 'David Miller'; +Cc: jakub.kicinski, netdev
> From: David Miller [mailto:davem@davemloft.net]
>
> From: "Prashant Bhole" <bhole_prashant_q7@lab.ntt.co.jp>
> Date: Tue, 19 Dec 2017 13:45:47 +0900
>
> > I tried to evaluate whether fixing return value of
> > debugfs_create_dir() (and
> > friends) will be useful or not because it has not been changed since
> > very long time. Now I am not much convinced about changing this api.
> >
> > Important and possible error codes could be -EEXIST and -ENOMEM.
> > Suppose -EEXIST is returned, IMO the directory shouldn't exists in the
> > first place because it is specific to particular module. Also, there
> > is no point in creating file in such directory, because directory
> > owner (creator) might remove it too. This means there are less chances
> > that api change will be useful. Please let me know your opinion on it.
> >
> > If you are ok with above explanation, shall I submit v2 for this patch?
>
> Well, something is seriously wrong if the directory exists already.
>
> It could be that two netdevsim modules, independantly compiled, are trying
to
> be loaded.
>
> Wouldn't it clearly be desirable to fail and not load the module in that
case?
>
> This is why I think ignoring debugfs errors is foolish.
Right. I am planning to do following (quoting previous mail), In debugfs
error will not be ignored in modules load.
-----------
Dave,
Thanks for comments. I will try to fix error handling in netdevsim first.
Jakub,
Let's decide with an example. The typical directory structure for netdevsim
interface is as below:
/sys/kernel/debug/netdevsim/sim0/bpf_bound_progs/
Please let me know if you are ok with following:
1) If debugfs_create_dir() fails in module_init, let's keep it fatal error
with corrected condition:
+ if (IS_ERR_OR_NULL(nsim_ddir))
+ return -ENOMEM;
2) In case sim0 or bpf_bound_progs are fail to create, we need to add
checks before creating any file in them.
-----------
Shall I submit v2?
-Prashant
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-20 0:16 ` Prashant Bhole
@ 2017-12-20 0:30 ` Jakub Kicinski
2017-12-20 0:38 ` Prashant Bhole
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2017-12-20 0:30 UTC (permalink / raw)
To: Prashant Bhole; +Cc: 'David Miller', netdev
On Wed, 20 Dec 2017 09:16:01 +0900, Prashant Bhole wrote:
> > From: David Miller [mailto:davem@davemloft.net]
> >
> > From: "Prashant Bhole" <bhole_prashant_q7@lab.ntt.co.jp>
> > Date: Tue, 19 Dec 2017 13:45:47 +0900
> >
> > > I tried to evaluate whether fixing return value of
> > > debugfs_create_dir() (and
> > > friends) will be useful or not because it has not been changed since
> > > very long time. Now I am not much convinced about changing this api.
> > >
> > > Important and possible error codes could be -EEXIST and -ENOMEM.
> > > Suppose -EEXIST is returned, IMO the directory shouldn't exists in the
> > > first place because it is specific to particular module. Also, there
> > > is no point in creating file in such directory, because directory
> > > owner (creator) might remove it too. This means there are less chances
> > > that api change will be useful. Please let me know your opinion on it.
> > >
> > > If you are ok with above explanation, shall I submit v2 for this patch?
> >
> > Well, something is seriously wrong if the directory exists already.
> >
> > It could be that two netdevsim modules, independantly compiled, are trying
> to
> > be loaded.
> >
> > Wouldn't it clearly be desirable to fail and not load the module in that
> case?
> >
> > This is why I think ignoring debugfs errors is foolish.
>
> Right. I am planning to do following (quoting previous mail), In debugfs
> error will not be ignored in modules load.
> -----------
> Dave,
> Thanks for comments. I will try to fix error handling in netdevsim first.
>
> Jakub,
> Let's decide with an example. The typical directory structure for netdevsim
> interface is as below:
> /sys/kernel/debug/netdevsim/sim0/bpf_bound_progs/
> Please let me know if you are ok with following:
>
> 1) If debugfs_create_dir() fails in module_init, let's keep it fatal error
> with corrected condition:
> + if (IS_ERR_OR_NULL(nsim_ddir))
> + return -ENOMEM;
Ack.
> 2) In case sim0 or bpf_bound_progs are fail to create, we need to add
> checks before creating any file in them.
What do you mean by "check before"? Checking if creation of each file
fails or not, or something different?
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-20 0:30 ` Jakub Kicinski
@ 2017-12-20 0:38 ` Prashant Bhole
2017-12-20 0:45 ` Jakub Kicinski
0 siblings, 1 reply; 15+ messages in thread
From: Prashant Bhole @ 2017-12-20 0:38 UTC (permalink / raw)
To: 'Jakub Kicinski'; +Cc: 'David Miller', netdev
> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>
> On Wed, 20 Dec 2017 09:16:01 +0900, Prashant Bhole wrote:
> > > From: David Miller [mailto:davem@davemloft.net]
> > >
> > > From: "Prashant Bhole" <bhole_prashant_q7@lab.ntt.co.jp>
> > > Date: Tue, 19 Dec 2017 13:45:47 +0900
> > >
> > > > I tried to evaluate whether fixing return value of
> > > > debugfs_create_dir() (and
> > > > friends) will be useful or not because it has not been changed
> > > > since very long time. Now I am not much convinced about changing
this api.
> > > >
> > > > Important and possible error codes could be -EEXIST and -ENOMEM.
> > > > Suppose -EEXIST is returned, IMO the directory shouldn't exists in
> > > > the first place because it is specific to particular module. Also,
> > > > there is no point in creating file in such directory, because
> > > > directory owner (creator) might remove it too. This means there
> > > > are less chances that api change will be useful. Please let me know
your
> opinion on it.
> > > >
> > > > If you are ok with above explanation, shall I submit v2 for this
patch?
> > >
> > > Well, something is seriously wrong if the directory exists already.
> > >
> > > It could be that two netdevsim modules, independantly compiled, are
> > > trying
> > to
> > > be loaded.
> > >
> > > Wouldn't it clearly be desirable to fail and not load the module in
> > > that
> > case?
> > >
> > > This is why I think ignoring debugfs errors is foolish.
> >
> > Right. I am planning to do following (quoting previous mail), In
> > debugfs error will not be ignored in modules load.
> > -----------
> > Dave,
> > Thanks for comments. I will try to fix error handling in netdevsim
first.
> >
> > Jakub,
> > Let's decide with an example. The typical directory structure for
> > netdevsim interface is as below:
> > /sys/kernel/debug/netdevsim/sim0/bpf_bound_progs/
> > Please let me know if you are ok with following:
> >
> > 1) If debugfs_create_dir() fails in module_init, let's keep it fatal
> > error with corrected condition:
> > + if (IS_ERR_OR_NULL(nsim_ddir))
> > + return -ENOMEM;
>
> Ack.
>
> > 2) In case sim0 or bpf_bound_progs are fail to create, we need to add
> > checks before creating any file in them.
>
> What do you mean by "check before"? Checking if creation of each file
fails or
> not, or something different?
For example:
I will check if state->ddir is not NULL before creating files in it.
if (state->ddir) {
debugfs_create_u32("id", 0400, state->ddir, &prog->aux->id);
debugfs_create_file("state", 0400, state->ddir,
&state->state, &nsim_bpf_string_fops);
debugfs_create_bool("loaded", 0400, state->ddir, &state->is_loaded);
}
-Prashant
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-20 0:38 ` Prashant Bhole
@ 2017-12-20 0:45 ` Jakub Kicinski
2017-12-20 0:54 ` Prashant Bhole
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2017-12-20 0:45 UTC (permalink / raw)
To: Prashant Bhole; +Cc: 'David Miller', netdev
On Wed, 20 Dec 2017 09:38:52 +0900, Prashant Bhole wrote:
> > > 2) In case sim0 or bpf_bound_progs are fail to create, we need to add
> > > checks before creating any file in them.
> >
> > What do you mean by "check before"? Checking if creation of each file
> > fails or not, or something different?
>
> For example:
> I will check if state->ddir is not NULL before creating files in it.
>
> if (state->ddir) {
> debugfs_create_u32("id", 0400, state->ddir, &prog->aux->id);
> debugfs_create_file("state", 0400, state->ddir,
> &state->state, &nsim_bpf_string_fops);
> debugfs_create_bool("loaded", 0400, state->ddir, &state->is_loaded);
> }
Ah, I would just error out in case we can't create any of the
sub-directories as well.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-20 0:45 ` Jakub Kicinski
@ 2017-12-20 0:54 ` Prashant Bhole
2017-12-20 1:18 ` Jakub Kicinski
0 siblings, 1 reply; 15+ messages in thread
From: Prashant Bhole @ 2017-12-20 0:54 UTC (permalink / raw)
To: 'Jakub Kicinski'; +Cc: 'David Miller', netdev
> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>
> On Wed, 20 Dec 2017 09:38:52 +0900, Prashant Bhole wrote:
> > > > 2) In case sim0 or bpf_bound_progs are fail to create, we need to
> > > > add checks before creating any file in them.
> > >
> > > What do you mean by "check before"? Checking if creation of each
> > > file fails or not, or something different?
> >
> > For example:
> > I will check if state->ddir is not NULL before creating files in it.
> >
> > if (state->ddir) {
> > debugfs_create_u32("id", 0400, state->ddir, &prog->aux->id);
> > debugfs_create_file("state", 0400, state->ddir,
> > &state->state, &nsim_bpf_string_fops);
> > debugfs_create_bool("loaded", 0400, state->ddir, &state->is_loaded);
> > }
>
> Ah, I would just error out in case we can't create any of the
sub-directories as
> well.
Does that mean fatal error if we can't create any of the subdirectories?
Or
Similar check as mentioned above before creating subdirectories? (I was
about to do this)
-Prashant
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-20 0:54 ` Prashant Bhole
@ 2017-12-20 1:18 ` Jakub Kicinski
2017-12-20 1:40 ` Prashant Bhole
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2017-12-20 1:18 UTC (permalink / raw)
To: Prashant Bhole; +Cc: 'David Miller', netdev
On Wed, 20 Dec 2017 09:54:59 +0900, Prashant Bhole wrote:
> > Ah, I would just error out in case we can't create any of the
> > sub-directories as well.
>
> Does that mean fatal error if we can't create any of the subdirectories?
Yes.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir
2017-12-20 1:18 ` Jakub Kicinski
@ 2017-12-20 1:40 ` Prashant Bhole
0 siblings, 0 replies; 15+ messages in thread
From: Prashant Bhole @ 2017-12-20 1:40 UTC (permalink / raw)
To: 'Jakub Kicinski'; +Cc: 'David Miller', netdev
> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>
> On Wed, 20 Dec 2017 09:54:59 +0900, Prashant Bhole wrote:
> > > Ah, I would just error out in case we can't create any of the
> > > sub-directories as well.
> >
> > Does that mean fatal error if we can't create any of the subdirectories?
>
> Yes.
Ok. In this case there is no need of condition before creating files. I will
submit v2.
-Prashant
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-12-20 1:41 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-08 0:52 [PATCH net-next] netdevsim: correctly check return value of debugfs_create_dir Prashant Bhole
2017-12-08 1:12 ` Jakub Kicinski
2017-12-08 2:12 ` Prashant Bhole
2017-12-08 15:31 ` David Miller
2017-12-11 4:46 ` Prashant Bhole
2017-12-11 18:40 ` Jakub Kicinski
2017-12-19 4:45 ` Prashant Bhole
2017-12-19 14:11 ` David Miller
2017-12-20 0:16 ` Prashant Bhole
2017-12-20 0:30 ` Jakub Kicinski
2017-12-20 0:38 ` Prashant Bhole
2017-12-20 0:45 ` Jakub Kicinski
2017-12-20 0:54 ` Prashant Bhole
2017-12-20 1:18 ` Jakub Kicinski
2017-12-20 1:40 ` Prashant Bhole
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).