Messages in this thread | | | Date | Tue, 2 Apr 2024 14:19:18 +0300 | Subject | Re: [PATCH 5/5] staging: pi433: Remove duplicated code using the "goto" error recovery scheme. | From | 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@gmail.com> >> --- >> 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
| |