Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep_init

[pmarzo <marzo.pedro@gmail.com>]

On mié, 2015-05-20 at 00:46 +0300, Dan Carpenter wrote:
> I was planning to leave this one for Greg but since you asked me to
> comment...
> 
> This patch is ok as one patch.  The subject is "clean up
> prism2_wep_init()" and that's one thing.  Breaking it up into tiny
> patches would probably make it harder to review.
> 
> On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> > Merge two pr_debug lines with literal strings splitted across several lines
> > into one single line, simplifying prism2_wep_init error check code.
> > 
> > Signed-off-by: Pedro Marzo Perez <marzo.pedro@gmail.com>
> > ---
> >  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 22 +++++++++-------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > index 0a17f84..388ed3c 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > @@ -9,6 +9,8 @@
> >   * more details.
> >   */
> >  
> > +#define pr_fmt(fmt) "ieee80211_crypt_wep: " fmt
> 
> Greg doesn't like pr_fmt() in driver code, use dev_dbg() and friends
> instead.  I don't like debug output generally so I say just delete it.
> Especially in this case the debug output is pretty useless.
dev_dbg needs the device parameter, I don't see a way to get the device
pointer within this function. Anyway I think you are right, this debug
output is not very useful, if Greg agrees I will delete the line.
I am curious, why you dont like debug output? I usually have to adapt
the kernel to run in ARM based boards and I find debug output really
useful.

> > +
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/slab.h>
> > @@ -43,20 +45,16 @@ static void *prism2_wep_init(int keyidx)
> >  
> >  	priv = kzalloc(sizeof(*priv), GFP_ATOMIC);
> >  	if (priv == NULL)
> > -		goto fail;
> > +		return NULL;
> >  	priv->key_idx = keyidx;
> >  
> >  	priv->tx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
> >  	if (IS_ERR(priv->tx_tfm)) {
> > -		pr_debug("ieee80211_crypt_wep: could not allocate "
> > -		       "crypto API arc4\n");
> >  		priv->tx_tfm = NULL;
> >  		goto fail;
> 
> This error handling is overly complicated.  It's supposed to be you
> climb up a mountain to go hang gliding (return zero), but if you hit an
> error on the way to the talk you walk backwards the way you came to the
> bottom of the mountain.  You see all the landmarks in reverse order and
> it makes you have a sad face.
Agree, it is still too complicated. The patch simplifies the original
code but it is still not completely right, and it does not follow kernel
error check style if error => goto

> 
> allocate priv
> allocate tx
> allocate rx
> 
> return success;  <-- made it to the top
> 
> free tx <- walking back down.  :(
> free priv
> 
> It should be:
> 
> 	if (IS_ERR(priv->tx_tfm))
> 		goto free_priv;
> 
> >  	}
> >  	priv->rx_tfm = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
> >  	if (IS_ERR(priv->rx_tfm)) {
> > -		pr_debug("ieee80211_crypt_wep: could not allocate "
> > -		       "crypto API arc4\n");
> >  		priv->rx_tfm = NULL;
> >  		goto fail;
> >  	}
> 
> 	if (IS_ERR(priv->rx_tfm))
> 		goto free_tx;
> 
> If this allocation succeeds then we've made it to the top.  No need to
> call crypto_free_blkcipher(priv->rx_tfm) in this function.
Agree, this check is useless, will never be called

> 
> > @@ -67,14 +65,12 @@ static void *prism2_wep_init(int keyidx)
> >  	return priv;
> >  
> >  fail:
> > -	if (priv) {
> > -		if (priv->tx_tfm)
> > -			crypto_free_blkcipher(priv->tx_tfm);
> > -		if (priv->rx_tfm)
> > -			crypto_free_blkcipher(priv->rx_tfm);
> > -		kfree(priv);
> > -	}
> 
> In the original code there isn't a nice error path down the mountain,
> it's all mixed together and not in any particular order.
> 
> > -
> > +	pr_debug("could not allocate crypto API arc4\n");
> > +	if (priv->tx_tfm)
> > +		crypto_free_blkcipher(priv->tx_tfm);
> > +	if (priv->rx_tfm)
> > +		crypto_free_blkcipher(priv->rx_tfm);
> > +	kfree(priv);
> >  	return NULL;
> 
> 
> free_tx:
> 	crypto_free_blkcipher(priv->tx_tfm);
> free_priv:
> 	kfree(priv);
> 
> 	return NULL;
> 

I will send an V4 patch, I am glad you don't charge a fee per patch
submitted and reviewed :-)
Anyway I will wait a couple of days to Greg's comments, so hopefully V4
will be the last.


> regards,
> dan carpenter