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

[Dan Carpenter <dan.carpenter@oracle.com>]

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.

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

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.

> @@ -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;

regards,
dan carpenter