Re: [PATCH 5/5] staging: pi433: Remove duplicated code using the "goto" error recovery scheme.

[Shahar Avidar]

Hi Dan, I appreciate your review.

On 28/03/2024 16:03, Dan Carpenter wrote:
> On Thu, Mar 28, 2024 at 02:12:44PM +0200, Shahar Avidar wrote:
> > pi433_init had "unregister_chrdev" called twice.
> > Remove it using goto statements.
> >
> > Signed-off-by: Shahar Avidar <ikobh7@xxxxxxxxx>
> > ---
> >   drivers/staging/pi433/pi433_if.c | 23 ++++++++++++-----------
> >   1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > index 62ce75b07bf0..e538f1d4e787 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -1400,24 +1400,25 @@ static int __init pi433_init(void)
> >   	 */
> >   	status = alloc_chrdev_region(&pi433_devt, 0, N_PI433_MINORS, "pi433");
> >   	if (status < 0)
> > -		return status;
> > +		goto fail;
>
> Just return directly.  Do nothing gotos are bad for readability and they
> create Forgot to Set the Error Code bugs.  Also there was a direct
> return earlier in the function so it's better to do things consistently.
Fixed in V2.

> >   
> >   	status = class_register(&pi433_class);
> > -	if (status) {
> > -		unregister_chrdev(MAJOR(pi433_devt),
> > -				  pi433_spi_driver.driver.name);
> > -		return status;
> > -	}
> > +	if (status)
> > +		goto unreg_chrdev;
> >   
> >   	root_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> >   
> >   	status = spi_register_driver(&pi433_spi_driver);
> > -	if (status < 0) {
> > -		class_unregister(&pi433_class);
> > -		unregister_chrdev(MAJOR(pi433_devt),
> > -				  pi433_spi_driver.driver.name);
> > -	}
> > +	if (status < 0)
> > +		goto unreg_class;
> >   
> > +	return 0;
> > +
> > +unreg_class:
> > +	class_unregister(&pi433_class);
>
> There is a debugfs_remove() missing.  I have written a blog that might
> be helpful:
>
> https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
Interesting blog post.
I hope I followed through & made the right changes.
> regards,
> dan carpenter
>
> > +unreg_chrdev:
> > +	unregister_chrdev(MAJOR(pi433_devt), pi433_spi_driver.driver.name);
> > +fail:
> >   	return status;
> >   }
--
Regards,

Shahar