* [PATCH] staging: wilc1000: merge 'if' statements that test the same condition
@ 2018-02-24 7:47 Dafna Hirschfeld
2018-02-24 8:03 ` [Outreachy kernel] " Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Dafna Hirschfeld @ 2018-02-24 7:47 UTC (permalink / raw)
To: aditya.shankar, ganesh.krishna, gregkh; +Cc: outreachy-kernel
Merge the instructions of two 'if' statements that test the same
condition and move a 'memcpy' instruction related to a different variable.
Issue found with coccicheck.
Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
---
drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index 621810d..f6f2c7f8 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -962,15 +962,13 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
priv->wilc_ptk[key_index]->key = kmalloc(params->key_len, GFP_KERNEL);
+ memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
kfree(priv->wilc_ptk[key_index]->seq);
- if (params->seq_len > 0)
+ if (params->seq_len > 0) {
priv->wilc_ptk[key_index]->seq = kmalloc(params->seq_len, GFP_KERNEL);
-
- memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
-
- if (params->seq_len > 0)
memcpy(priv->wilc_ptk[key_index]->seq, params->seq, params->seq_len);
+ }
priv->wilc_ptk[key_index]->cipher = params->cipher;
priv->wilc_ptk[key_index]->key_len = params->key_len;
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: wilc1000: merge 'if' statements that test the same condition
2018-02-24 7:47 [PATCH] staging: wilc1000: merge 'if' statements that test the same condition Dafna Hirschfeld
@ 2018-02-24 8:03 ` Julia Lawall
2018-02-24 9:02 ` Dafna Hirschfeld
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2018-02-24 8:03 UTC (permalink / raw)
To: Dafna Hirschfeld; +Cc: aditya.shankar, ganesh.krishna, gregkh, outreachy-kernel
On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
> Merge the instructions of two 'if' statements that test the same
> condition and move a 'memcpy' instruction related to a different variable.
> Issue found with coccicheck.
>
> Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
> ---
> drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index 621810d..f6f2c7f8 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -962,15 +962,13 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
>
> priv->wilc_ptk[key_index]->key = kmalloc(params->key_len, GFP_KERNEL);
>
> + memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> kfree(priv->wilc_ptk[key_index]->seq);
How about moving the memcpy up a line to be next to the kmalloc, and to be
separated by a blank line from the kfree, which is on a different
location.
These kmallocs should also have null tests to check for failure. That
could be the first patch in the series with this patch being the second
one. The missing null checks represent a small bug, and a bug should not
depend on a cleanup.
julia
>
> - if (params->seq_len > 0)
> + if (params->seq_len > 0) {
> priv->wilc_ptk[key_index]->seq = kmalloc(params->seq_len, GFP_KERNEL);
> -
> - memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> -
> - if (params->seq_len > 0)
> memcpy(priv->wilc_ptk[key_index]->seq, params->seq, params->seq_len);
> + }
>
> priv->wilc_ptk[key_index]->cipher = params->cipher;
> priv->wilc_ptk[key_index]->key_len = params->key_len;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20180224074733.GA6487%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: wilc1000: merge 'if' statements that test the same condition
2018-02-24 8:03 ` [Outreachy kernel] " Julia Lawall
@ 2018-02-24 9:02 ` Dafna Hirschfeld
2018-02-24 9:38 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Dafna Hirschfeld @ 2018-02-24 9:02 UTC (permalink / raw)
To: Julia Lawall; +Cc: aditya.shankar, ganesh.krishna, gregkh, outreachy-kernel
On Sat, Feb 24, 2018 at 09:03:20AM +0100, Julia Lawall wrote:
>
>
> On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
>
> > Merge the instructions of two 'if' statements that test the same
> > condition and move a 'memcpy' instruction related to a different variable.
> > Issue found with coccicheck.
> >
> > Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
> > ---
> > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 +++-----
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > index 621810d..f6f2c7f8 100644
> > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > @@ -962,15 +962,13 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
> >
> > priv->wilc_ptk[key_index]->key = kmalloc(params->key_len, GFP_KERNEL);
> >
> > + memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> > kfree(priv->wilc_ptk[key_index]->seq);
>
> How about moving the memcpy up a line to be next to the kmalloc, and to be
> separated by a blank line from the kfree, which is on a different
> location.
>
> These kmallocs should also have null tests to check for failure. That
> could be the first patch in the series with this patch being the second
> one. The missing null checks represent a small bug, and a bug should not
> depend on a cleanup.
>
> julia
>
Hi, the file has about 17 calls to kmalloc that are not tested for null.
Wouldn't it be better to fix all of them in a different patch?
Dafna
> >
> > - if (params->seq_len > 0)
> > + if (params->seq_len > 0) {
> > priv->wilc_ptk[key_index]->seq = kmalloc(params->seq_len, GFP_KERNEL);
> > -
> > - memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> > -
> > - if (params->seq_len > 0)
> > memcpy(priv->wilc_ptk[key_index]->seq, params->seq, params->seq_len);
> > + }
> >
> > priv->wilc_ptk[key_index]->cipher = params->cipher;
> > priv->wilc_ptk[key_index]->key_len = params->key_len;
> > --
> > 2.7.4
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20180224074733.GA6487%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: wilc1000: merge 'if' statements that test the same condition
2018-02-24 9:02 ` Dafna Hirschfeld
@ 2018-02-24 9:38 ` Julia Lawall
2018-02-25 6:06 ` Dafna Hirschfeld
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2018-02-24 9:38 UTC (permalink / raw)
To: Dafna Hirschfeld; +Cc: aditya.shankar, ganesh.krishna, gregkh, outreachy-kernel
On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
> On Sat, Feb 24, 2018 at 09:03:20AM +0100, Julia Lawall wrote:
> >
> >
> > On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
> >
> > > Merge the instructions of two 'if' statements that test the same
> > > condition and move a 'memcpy' instruction related to a different variable.
> > > Issue found with coccicheck.
> > >
> > > Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
> > > ---
> > > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 +++-----
> > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > index 621810d..f6f2c7f8 100644
> > > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > @@ -962,15 +962,13 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
> > >
> > > priv->wilc_ptk[key_index]->key = kmalloc(params->key_len, GFP_KERNEL);
> > >
> > > + memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> > > kfree(priv->wilc_ptk[key_index]->seq);
> >
> > How about moving the memcpy up a line to be next to the kmalloc, and to be
> > separated by a blank line from the kfree, which is on a different
> > location.
> >
> > These kmallocs should also have null tests to check for failure. That
> > could be the first patch in the series with this patch being the second
> > one. The missing null checks represent a small bug, and a bug should not
> > depend on a cleanup.
> >
> > julia
> >
> Hi, the file has about 17 calls to kmalloc that are not tested for null.
> Wouldn't it be better to fix all of them in a different patch?
Sure. It's just that your choices are either to wait for Greg to take one
patch before taking the next, or put them in a series.
But maybe it would be just as well to get this one out of the way. The
degree of difficulty is quite different, because to add error handlig
code, you have to figure out how to handle the error. So that needs to be
done slowly and carefully.
julia
>
> Dafna
>
> > >
> > > - if (params->seq_len > 0)
> > > + if (params->seq_len > 0) {
> > > priv->wilc_ptk[key_index]->seq = kmalloc(params->seq_len, GFP_KERNEL);
> > > -
> > > - memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> > > -
> > > - if (params->seq_len > 0)
> > > memcpy(priv->wilc_ptk[key_index]->seq, params->seq, params->seq_len);
> > > + }
> > >
> > > priv->wilc_ptk[key_index]->cipher = params->cipher;
> > > priv->wilc_ptk[key_index]->key_len = params->key_len;
> > > --
> > > 2.7.4
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20180224074733.GA6487%40gmail.com.
> > > For more options, visit https://groups.google.com/d/optout.
> > >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: wilc1000: merge 'if' statements that test the same condition
2018-02-24 9:38 ` Julia Lawall
@ 2018-02-25 6:06 ` Dafna Hirschfeld
2018-02-25 8:50 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Dafna Hirschfeld @ 2018-02-25 6:06 UTC (permalink / raw)
To: Julia Lawall; +Cc: outreachy-kernel, gregkh, aditya.shankar, ganesh.krishna
On Sat, Feb 24, 2018 at 10:38:55AM +0100, Julia Lawall wrote:
>
>
> On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
>
> > On Sat, Feb 24, 2018 at 09:03:20AM +0100, Julia Lawall wrote:
> > >
> > >
> > > On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
> > >
> > > > Merge the instructions of two 'if' statements that test the same
> > > > condition and move a 'memcpy' instruction related to a different variable.
> > > > Issue found with coccicheck.
> > > >
> > > > Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
> > > > ---
> > > > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 +++-----
> > > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > > index 621810d..f6f2c7f8 100644
> > > > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > > @@ -962,15 +962,13 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
> > > >
> > > > priv->wilc_ptk[key_index]->key = kmalloc(params->key_len, GFP_KERNEL);
> > > >
> > > > + memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> > > > kfree(priv->wilc_ptk[key_index]->seq);
> > >
> > > How about moving the memcpy up a line to be next to the kmalloc, and to be
> > > separated by a blank line from the kfree, which is on a different
> > > location.
> > >
> > > These kmallocs should also have null tests to check for failure. That
> > > could be the first patch in the series with this patch being the second
> > > one. The missing null checks represent a small bug, and a bug should not
> > > depend on a cleanup.
> > >
> > > julia
> > >
> > Hi, the file has about 17 calls to kmalloc that are not tested for null.
> > Wouldn't it be better to fix all of them in a different patch?
>
> Sure. It's just that your choices are either to wait for Greg to take one
> patch before taking the next, or put them in a series.
>
> But maybe it would be just as well to get this one out of the way. The
> degree of difficulty is quite different, because to add error handlig
> code, you have to figure out how to handle the error. So that needs to be
> done slowly and carefully.
>
Hi, regarding adding null checks to kmalloc calls.
I wonder if it is possible to do it without deeply know the whole picture of
what the code does and with what other code it interacts.
I saw that there are kmalloc calls from a function 'add_network_to_shadow'
which is part of a code that enters a working queue. The other calls are from
functions that implement callbacks of 'struct cfg80211_ops'.
I saw that other drivers that implemets these callbacks return -ENOMEM.
In case of kmalloc returning null, is it enough to free everything else
that was allocated in the function and return -ENOMEM?
Dafna
> julia
>
> >
> > Dafna
> >
> > > >
> > > > - if (params->seq_len > 0)
> > > > + if (params->seq_len > 0) {
> > > > priv->wilc_ptk[key_index]->seq = kmalloc(params->seq_len, GFP_KERNEL);
> > > > -
> > > > - memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> > > > -
> > > > - if (params->seq_len > 0)
> > > > memcpy(priv->wilc_ptk[key_index]->seq, params->seq, params->seq_len);
> > > > + }
> > > >
> > > > priv->wilc_ptk[key_index]->cipher = params->cipher;
> > > > priv->wilc_ptk[key_index]->key_len = params->key_len;
> > > > --
> > > > 2.7.4
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20180224074733.GA6487%40gmail.com.
> > > > For more options, visit https://groups.google.com/d/optout.
> > > >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Outreachy kernel] [PATCH] staging: wilc1000: merge 'if' statements that test the same condition
2018-02-25 6:06 ` Dafna Hirschfeld
@ 2018-02-25 8:50 ` Julia Lawall
0 siblings, 0 replies; 6+ messages in thread
From: Julia Lawall @ 2018-02-25 8:50 UTC (permalink / raw)
To: Dafna Hirschfeld; +Cc: outreachy-kernel, gregkh, aditya.shankar, ganesh.krishna
On Sun, 25 Feb 2018, Dafna Hirschfeld wrote:
> On Sat, Feb 24, 2018 at 10:38:55AM +0100, Julia Lawall wrote:
> >
> >
> > On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
> >
> > > On Sat, Feb 24, 2018 at 09:03:20AM +0100, Julia Lawall wrote:
> > > >
> > > >
> > > > On Sat, 24 Feb 2018, Dafna Hirschfeld wrote:
> > > >
> > > > > Merge the instructions of two 'if' statements that test the same
> > > > > condition and move a 'memcpy' instruction related to a different variable.
> > > > > Issue found with coccicheck.
> > > > >
> > > > > Signed-off-by: Dafna Hirschfeld <dafna3@gmail.com>
> > > > > ---
> > > > > drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 8 +++-----
> > > > > 1 file changed, 3 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > > > index 621810d..f6f2c7f8 100644
> > > > > --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > > > +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> > > > > @@ -962,15 +962,13 @@ static int add_key(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
> > > > >
> > > > > priv->wilc_ptk[key_index]->key = kmalloc(params->key_len, GFP_KERNEL);
> > > > >
> > > > > + memcpy(priv->wilc_ptk[key_index]->key, params->key, params->key_len);
> > > > > kfree(priv->wilc_ptk[key_index]->seq);
> > > >
> > > > How about moving the memcpy up a line to be next to the kmalloc, and to be
> > > > separated by a blank line from the kfree, which is on a different
> > > > location.
> > > >
> > > > These kmallocs should also have null tests to check for failure. That
> > > > could be the first patch in the series with this patch being the second
> > > > one. The missing null checks represent a small bug, and a bug should not
> > > > depend on a cleanup.
> > > >
> > > > julia
> > > >
> > > Hi, the file has about 17 calls to kmalloc that are not tested for null.
> > > Wouldn't it be better to fix all of them in a different patch?
> >
> > Sure. It's just that your choices are either to wait for Greg to take one
> > patch before taking the next, or put them in a series.
> >
> > But maybe it would be just as well to get this one out of the way. The
> > degree of difficulty is quite different, because to add error handlig
> > code, you have to figure out how to handle the error. So that needs to be
> > done slowly and carefully.
> >
> Hi, regarding adding null checks to kmalloc calls.
> I wonder if it is possible to do it without deeply know the whole picture of
> what the code does and with what other code it interacts.
> I saw that there are kmalloc calls from a function 'add_network_to_shadow'
> which is part of a code that enters a working queue. The other calls are from
> functions that implement callbacks of 'struct cfg80211_ops'.
> I saw that other drivers that implemets these callbacks return -ENOMEM.
> In case of kmalloc returning null, is it enough to free everything else
> that was allocated in the function and return -ENOMEM?
-ENOMEM is very standard for kmalloc, so it is probably fine. Maybe look
around elsewhere in the function to see what frees are done, to see what
would be adapted to each situation. If it looks like each case will be
different, don't feel obliged to do them all. Indeed, unless they are all
quite the same, it might be better to do them one by one. Then if a
mistake is found later in one of them, it will be possible to revert that
patch without reverting all of them.
Your commit message should carefully explain your reasoning in each case.
julia
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-25 8:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-24 7:47 [PATCH] staging: wilc1000: merge 'if' statements that test the same condition Dafna Hirschfeld
2018-02-24 8:03 ` [Outreachy kernel] " Julia Lawall
2018-02-24 9:02 ` Dafna Hirschfeld
2018-02-24 9:38 ` Julia Lawall
2018-02-25 6:06 ` Dafna Hirschfeld
2018-02-25 8:50 ` Julia Lawall
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.