On Thu, Dec 16, 2010 at 01:26:58PM -0600, William Hubbs wrote:
> fix the failure handling in kobjects and the main function so that we
> release the virtual keyboard if we exit due to another failure.
>
> Signed-off-by: William Hubbs <w.d.hubbs@xxxxxxxxx>
> ---
> drivers/staging/speakup/kobjects.c | 9 ++++++---
> drivers/staging/speakup/main.c | 33 +++++++++++++++++++++------------
> 2 files changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
> index cc79f9e..408bb9b 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
The changes in this file are just white space changes. It's too late
in the release cycle for whitespace changes.
Also the original code is better than the new code. You've added gotos
to try have only one return statement per function. But really
pointless gotos are annoying because I was expecting the gotos to do
something but they didn't.
Kernel style is to write the minimum amount of code. Having a success
path and a unwind path is normal. Don't over engineer.
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index 4b7a9c2..3cd0039 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -2253,17 +2253,17 @@ static int __init speakup_init(void)
>
> err = speakup_add_virtual_keyboard();
> if (err)
> - return err;
> + goto out;
Leave this return as is.
>
> initialize_msgs(); /* Initialize arrays for i18n. */
> first_console = kzalloc(sizeof(*first_console), GFP_KERNEL);
> - if (!first_console)
> - return -ENOMEM;
> - err = speakup_kobj_init();
> - if (err) {
> - kfree(first_console);
> - return err;
> + if (!first_console) {
> + err = -ENOMEM;
> + goto err_cons;
It's better that your labels describe the destination instead of where
we are now. If you add a second "goto err_cons" later, the name doesn't
describe anything meaningful:
if (x == 42)
goto err_cons; /* what? */
But if your label describes the destination it looks like this:
if (foo == bar)
goto err_remove_keyboard; /* <- very informative */
> }
> + err = speakup_kobj_init();
> + if (err)
> + goto err_kobject;
This goto is wrong. The init failed so the call to speakup_kobj_exit()
will cause an OOPs.
>
> reset_default_chars();
> reset_default_chartab();
> @@ -2299,11 +2299,20 @@ static int __init speakup_init(void)
>
> speakup_task = kthread_create(speakup_thread, NULL, "speakup");
> set_user_nice(speakup_task, 10);
> - if (!IS_ERR(speakup_task))
> - wake_up_process(speakup_task);
> - else
> - return -ENOMEM;
> - return 0;
> + if (IS_ERR(speakup_task)) {
> + err = -ENOMEM;
This should be:
err = PTR_ERR(speakup_task);
> + goto err_kobject;
> + }
> + wake_up_process(speakup_task);
> + goto out;
return 0;
> +
> +err_kobject:
> +speakup_kobj_exit();
^^^
The indenting here isn't right.
> + kfree(first_console);
> +err_cons:
> + speakup_remove_virtual_keyboard();
> +out:
> + return err;
> }
Sorry, this was like a way nit-picky review. I would normally not
have responded about things like naming labels but there was a NULL
deref in there and since you were going to redo it anyway I decided
to add in the bonus commentary.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
