Re: [Outreachy kernel] [PATCH] staging: wilc1000: merge 'if' statements that test the same condition

[Dafna Hirschfeld <dafna3@gmail.com>]

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