Re: [PATCH] staging: bcm: fix error handling in bcm_init()

[Kevin McKinney]

On Sun, Sep 02, 2012 at 01:37:36AM +0400, Alexey Khoroshilov wrote:
> bcm_init() does not have proper error handling of usb_register().
> The patch implements one.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
> ---
>  drivers/staging/bcm/InterfaceInit.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/bcm/InterfaceInit.c b/drivers/staging/bcm/InterfaceInit.c
> index 8f85de6..0049890 100644
> --- a/drivers/staging/bcm/InterfaceInit.c
> +++ b/drivers/staging/bcm/InterfaceInit.c
> @@ -669,6 +669,8 @@ struct class *bcm_class;
>  
>  static __init int bcm_init(void)
>  {
> +	int retval;
> +
>  	printk(KERN_INFO "%s: %s, %s\n", DRV_NAME, DRV_DESCRIPTION, DRV_VERSION);
>  	printk(KERN_INFO "%s\n", DRV_COPYRIGHT);
>  
> @@ -678,7 +680,15 @@ static __init int bcm_init(void)
>  		return PTR_ERR(bcm_class);
>  	}
>  
> -	return usb_register(&usbbcm_driver);
> +	retval = usb_register(&usbbcm_driver);
> +	if (retval < 0) {
> +		printk(KERN_ERR DRV_NAME ": could not register usb driver\n");
> +		goto out_class;
> +	}
> +	return 0;
> +out_class:
> +	class_destroy(bcm_class);
> +	return retval;
>  }
>  
Hi Alexey,

Instead of using a goto statement, can we just: (1) destory the class and (2) return retval within the if statement? i.e:

if (retval < 0) {
	printk(KERN_ERR DRV_NAME ": could not register usb driver\n");
	+class_destroy(bcm_class);
	+return retval;
}

goto statements are very useful and handy when a function exits from multiple locations because they provide a 
common exit point. The compiler will translate this into a "jump" instruction. However in this case I am not sure
it is simplifying our logic because we only have one usage. lets just call the two lines of code directly within the
if statement.

In addition, while you are in this code can you replace the "printk" statements with the preferred "pr_info" in a separate patch?  

Thanks,
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/