The functions debug_unregister() and kfree_fsm() test whether their argument
is NULL and then return immediately. Thus the test around the call
is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/s390/net/claw.c | 6 ++----
drivers/s390/net/ctcm_main.c | 6 ++----
drivers/s390/net/lcs.c | 6 ++----
drivers/s390/net/netiucv.c | 12 ++++--------
4 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/drivers/s390/net/claw.c b/drivers/s390/net/claw.c
index 213e54e..d609ca0 100644
--- a/drivers/s390/net/claw.c
+++ b/drivers/s390/net/claw.c
@@ -109,10 +109,8 @@ static debug_info_t *claw_dbf_trace;
static void
claw_unregister_debug_facility(void)
{
- if (claw_dbf_setup)
- debug_unregister(claw_dbf_setup);
- if (claw_dbf_trace)
- debug_unregister(claw_dbf_trace);
+ debug_unregister(claw_dbf_setup);
+ debug_unregister(claw_dbf_trace);
}
static int
diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index e056dd4..34dc0f3 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -1074,8 +1074,7 @@ static void ctcm_free_netdevice(struct net_device *dev)
if (priv) {
grp = priv->mpcg;
if (grp) {
- if (grp->fsm)
- kfree_fsm(grp->fsm);
+ kfree_fsm(grp->fsm);
if (grp->xid_skb)
dev_kfree_skb(grp->xid_skb);
if (grp->rcvd_xid_skb)
@@ -1672,8 +1671,7 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
ctcm_free_netdevice(dev);
}
- if (priv->fsm)
- kfree_fsm(priv->fsm);
+ kfree_fsm(priv->fsm);
ccw_device_set_offline(cgdev->cdev[1]);
ccw_device_set_offline(cgdev->cdev[0]);
diff --git a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
index 0a7d87c..5dfa7dd 100644
--- a/drivers/s390/net/lcs.c
+++ b/drivers/s390/net/lcs.c
@@ -88,10 +88,8 @@ static debug_info_t *lcs_dbf_trace;
static void
lcs_unregister_debug_facility(void)
{
- if (lcs_dbf_setup)
- debug_unregister(lcs_dbf_setup);
- if (lcs_dbf_trace)
- debug_unregister(lcs_dbf_trace);
+ debug_unregister(lcs_dbf_setup);
+ debug_unregister(lcs_dbf_trace);
}
static int
diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
index 0a87809..bdcc3fe 100644
--- a/drivers/s390/net/netiucv.c
+++ b/drivers/s390/net/netiucv.c
@@ -487,12 +487,9 @@ DEFINE_PER_CPU(char[256], iucv_dbf_txt_buf);
static void iucv_unregister_dbf_views(void)
{
- if (iucv_dbf_setup)
- debug_unregister(iucv_dbf_setup);
- if (iucv_dbf_data)
- debug_unregister(iucv_dbf_data);
- if (iucv_dbf_trace)
- debug_unregister(iucv_dbf_trace);
+ debug_unregister(iucv_dbf_setup);
+ debug_unregister(iucv_dbf_data);
+ debug_unregister(iucv_dbf_trace);
}
static int iucv_register_dbf_views(void)
{
@@ -1975,8 +1972,7 @@ static void netiucv_free_netdevice(struct net_device *dev)
if (privptr) {
if (privptr->conn)
netiucv_remove_connection(privptr->conn);
- if (privptr->fsm)
- kfree_fsm(privptr->fsm);
+ kfree_fsm(privptr->fsm);
privptr->conn = NULL; privptr->fsm = NULL;
/* privptr gets freed by free_netdev() */
}
--
2.1.3
This one is buggy.
I'm sorry, but please stop sending these.
For kfree(), at least we all know that kfree() accepts NULL pointer.
But for this one:
1) I don't know what the functions do so I have to look at the code.
2) It's in a arch that I don't compile so cscope isn't set up meaning
it's hard to find the functions.
You're sending a lot of patches and they are all hard to review and some
of them are buggy and none of them really add any value. It's a waste
of your time and it's a waste of my time.
regards,
dan carpenter
I agree with your proposed debug_unregister() changes, but not with your
kfree_fsm() change.
Regards, Ursula Braun
On Fri, 2014-10-31 at 18:40 +0100, SF Markus Elfring wrote:
> The functions debug_unregister() and kfree_fsm() test whether their argument
> is NULL and then return immediately. Thus the test around the call
> is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/s390/net/claw.c | 6 ++----
> drivers/s390/net/ctcm_main.c | 6 ++----
> drivers/s390/net/lcs.c | 6 ++----
> drivers/s390/net/netiucv.c | 12 ++++--------
> 4 files changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/s390/net/claw.c b/drivers/s390/net/claw.c
> index 213e54e..d609ca0 100644
> --- a/drivers/s390/net/claw.c
> +++ b/drivers/s390/net/claw.c
> @@ -109,10 +109,8 @@ static debug_info_t *claw_dbf_trace;
> static void
> claw_unregister_debug_facility(void)
> {
> - if (claw_dbf_setup)
> - debug_unregister(claw_dbf_setup);
> - if (claw_dbf_trace)
> - debug_unregister(claw_dbf_trace);
> + debug_unregister(claw_dbf_setup);
> + debug_unregister(claw_dbf_trace);
> }
>
> static int
> diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
> index e056dd4..34dc0f3 100644
> --- a/drivers/s390/net/ctcm_main.c
> +++ b/drivers/s390/net/ctcm_main.c
> @@ -1074,8 +1074,7 @@ static void ctcm_free_netdevice(struct net_device *dev)
> if (priv) {
> grp = priv->mpcg;
> if (grp) {
> - if (grp->fsm)
> - kfree_fsm(grp->fsm);
> + kfree_fsm(grp->fsm);
> if (grp->xid_skb)
> dev_kfree_skb(grp->xid_skb);
> if (grp->rcvd_xid_skb)
> @@ -1672,8 +1671,7 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
> ctcm_free_netdevice(dev);
> }
>
> - if (priv->fsm)
> - kfree_fsm(priv->fsm);
> + kfree_fsm(priv->fsm);
>
> ccw_device_set_offline(cgdev->cdev[1]);
> ccw_device_set_offline(cgdev->cdev[0]);
> diff --git a/drivers/s390/net/lcs.c b/drivers/s390/net/lcs.c
> index 0a7d87c..5dfa7dd 100644
> --- a/drivers/s390/net/lcs.c
> +++ b/drivers/s390/net/lcs.c
> @@ -88,10 +88,8 @@ static debug_info_t *lcs_dbf_trace;
> static void
> lcs_unregister_debug_facility(void)
> {
> - if (lcs_dbf_setup)
> - debug_unregister(lcs_dbf_setup);
> - if (lcs_dbf_trace)
> - debug_unregister(lcs_dbf_trace);
> + debug_unregister(lcs_dbf_setup);
> + debug_unregister(lcs_dbf_trace);
> }
>
> static int
> diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c
> index 0a87809..bdcc3fe 100644
> --- a/drivers/s390/net/netiucv.c
> +++ b/drivers/s390/net/netiucv.c
> @@ -487,12 +487,9 @@ DEFINE_PER_CPU(char[256], iucv_dbf_txt_buf);
>
> static void iucv_unregister_dbf_views(void)
> {
> - if (iucv_dbf_setup)
> - debug_unregister(iucv_dbf_setup);
> - if (iucv_dbf_data)
> - debug_unregister(iucv_dbf_data);
> - if (iucv_dbf_trace)
> - debug_unregister(iucv_dbf_trace);
> + debug_unregister(iucv_dbf_setup);
> + debug_unregister(iucv_dbf_data);
> + debug_unregister(iucv_dbf_trace);
> }
> static int iucv_register_dbf_views(void)
> {
> @@ -1975,8 +1972,7 @@ static void netiucv_free_netdevice(struct net_device *dev)
> if (privptr) {
> if (privptr->conn)
> netiucv_remove_connection(privptr->conn);
> - if (privptr->fsm)
> - kfree_fsm(privptr->fsm);
> + kfree_fsm(privptr->fsm);
> privptr->conn = NULL; privptr->fsm = NULL;
> /* privptr gets freed by free_netdev() */
> }
> This one is buggy.
I am still interested to clarify this opinion a bit more.
> I'm sorry, but please stop sending these.
I am going to improve more implementation details in affected source files.
> But for this one:
> 1) I don't know what the functions do so I have to look at the code.
I hope that static source code analysis can help here.
> 2) It's in a arch that I don't compile so cscope isn't set up meaning
> it's hard to find the functions.
Do you find the Coccinelle software also useful for your area?
> You're sending a lot of patches and they are all hard to review and some
> of them are buggy and none of them really add any value.
Thanks for your feedback.
The suggested source code clean-up might result in a measurable effect
depending on the call frequency for the changed functions.
Can I help you in any ways to make corresponding review easier?
> It's a waste of your time and it's a waste of my time.
It can be your choice to reject my update suggestion.
Regards,
Markus
> I agree with your proposed debug_unregister() changes, but not with your
> kfree_fsm() change.
Why do you want to keep an additional null pointer check before the call
of the kfree_fsm() function within the implementation of the
netiucv_free_netdevice() function?
Regards,
Markus
On Mon, Nov 03, 2014 at 04:55:12PM +0100, SF Markus Elfring wrote:
> > This one is buggy.
>
> I am still interested to clarify this opinion a bit more.
>
After your patch then it will print warning messages.
The truth is I think that all these patches are bad and they make the
code harder to read.
Before: The code is clear and there is no NULL dereference.
After: You have to remember that rtw_free_netdev() accepts NULL
pointers but free_netdev() does not accept NULL pointers.
The if statements are there for *human* readers to understand and you are
making it harder for humans to understand the code.
Even for kfree(), just removing the if statement is not really the right
fix. We do it because everyone knows kfree(), but what Julia Lawall
said is the real correct way change the code and make it simpler for
people to understand:
https://lkml.org/lkml/2014/10/31/452
I know it's fun to send automated patches but these make the code worse
and they waste reviewer time.
regards,
dan carpenter
On Mon, Nov 03, 2014 at 05:10:35PM +0100, SF Markus Elfring wrote:
> > I agree with your proposed debug_unregister() changes, but not with your
> > kfree_fsm() change.
>
> Why do you want to keep an additional null pointer check before the call
> of the kfree_fsm() function within the implementation of the
> netiucv_free_netdevice() function?
Think about how long it takes you to figure this out what the bug is and
then remember that we have to spend that same amount of time multiplied
by the number of patches you have sent.
regards,
dan carpenter
> After your patch then it will print warning messages.
To which messages do you refer to?
> The truth is I think that all these patches are bad and they make the
> code harder to read.
>
> Before: The code is clear and there is no NULL dereference.
Where do you stumble on a null pointer access?
> After: You have to remember that rtw_free_netdev() accepts NULL
> pointers but free_netdev() does not accept NULL pointers.
Are any improvements needed for the corresponding documentation to make it
better accessible besides the source code?
> The if statements are there for *human* readers to understand and you are
> making it harder for humans to understand the code.
Is there a target conflict between source code understandability
and software efficiency?
> Even for kfree(), just removing the if statement is not really the right
> fix. We do it because everyone knows kfree(), but what Julia Lawall
> said is the real correct way change the code and make it simpler for
> people to understand:
>
> https://lkml.org/lkml/2014/10/31/452
You refer to another update suggestion for the software area
"staging: rtl8188eu".
Do you find adjustments for jump labels easier to accept than the simple
deletion of specific null pointer checks?
> I know it's fun to send automated patches but these make the code worse
> and they waste reviewer time.
I hope that small automated changes can also help to improve affected
source files.
Regards,
Markus
> > After your patch then it will print warning messages.
> > After: You have to remember that rtw_free_netdev() accepts NULL
> > pointers but free_netdev() does not accept NULL pointers.
>
> Are any improvements needed for the corresponding documentation to make it
> better accessible besides the source code?
When people are writing or reading code, they will not necessarily look at
the documentation for every function that they use.
> > The if statements are there for *human* readers to understand and you are
> > making it harder for humans to understand the code.
>
> Is there a target conflict between source code understandability
> and software efficiency?
Efficiency is not an issue. This code is all in rare error handling paths
or in service removal functions. None of it is in a critical path. What
is important is to be able to easily check that what needs to be done is
actually done. Removing null tests makes it more obscure what needs to be
done, because it means that the conditions under which a function needs to
be called (which may be different than the conditions under which it can
be called) are less apparent.
julia
On Mon, Nov 03, 2014 at 05:50:48PM +0100, SF Markus Elfring wrote:
> > After your patch then it will print warning messages.
>
> To which messages do you refer to?
>
Open your eyeballs up and read the code.
>
> > The truth is I think that all these patches are bad and they make the
> > code harder to read.
> >
> > Before: The code is clear and there is no NULL dereference.
>
> Where do you stumble on a null pointer access?
>
I'm not talking about bugs, I'm talking about code clarity. Before is
more clear than after.
>
> > After: You have to remember that rtw_free_netdev() accepts NULL
> > pointers but free_netdev() does not accept NULL pointers.
>
> Are any improvements needed for the corresponding documentation to make it
> better accessible besides the source code?
>
Documentation doesn't reduce the number of things to remember it just
documents it. Meanwhile if we leave the code as-is there is no need for
documentation because the code is clear.
>
> > The if statements are there for *human* readers to understand and you are
> > making it harder for humans to understand the code.
>
> Is there a target conflict between source code understandability
> and software efficiency?
If you can benchmark the code and the new code is faster then, yes, this
patch is good and we will apply it. If you have no benchmarks then do
not send the patch.
>
> > Even for kfree(), just removing the if statement is not really the right
> > fix. We do it because everyone knows kfree(), but what Julia Lawall
> > said is the real correct way change the code and make it simpler for
> > people to understand:
> >
> > https://lkml.org/lkml/2014/10/31/452
>
> You refer to another update suggestion for the software area
> "staging: rtl8188eu".
> Do you find adjustments for jump labels easier to accept than the simple
> deletion of specific null pointer checks?
Yes.
>
>
> > I know it's fun to send automated patches but these make the code worse
> > and they waste reviewer time.
>
> I hope that small automated changes can also help to improve affected
> source files.
No. The changes make the code less clear.
regards,
dan carpenter
> If you can benchmark the code and the new code is faster then, yes, this
> patch is good and we will apply it.
I guess that I do not have enough resources myself to measure different run time
effects in a S390 environment.
> If you have no benchmarks then do not send the patch.
Are other software developers and testers eventually interested to try a few
pointer check adjustments out a bit more?
Regards,
Markus