* [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.