Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()

[DaeSeok Youn <daeseok.youn@gmail.com>]

Hi, Dan

Please look at below my line comment.

2014-05-28 16:02 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> We are talking about different things I think.  What I'm saying is that
> there is the normal way to do error handling in the kernel.  That's with
> a series of labels like this:
>
>         ...
>         return 0;
>
> err_free_ttys:
>         free_ttys();
> err_free_channels:
>         free_channels();
> err_free_brd:
>         free_brd();
>
>         return ret;
>
> In this code there are no if statements unless absolutely needed because
> of an matching if statement in the allocation code.  The label names
> describe what happens at the label.  It is in reverse order from the way
> the variables were allocated.
>
> The other thing is that at the end of dgap_tty_register() we have
> unwinding like this.
>
>   1304  unregister_serial_drv:
>   1305          tty_unregister_driver(brd->serial_driver);
>   1306  free_print_drv:
>   1307          put_tty_driver(brd->print_driver);
>   1308  free_serial_drv:
>   1309          put_tty_driver(brd->serial_driver);
>
> We can add a tty_unregister_driver(brd->print_driver) and create a
> dgap_tty_unregister().
>
> static void dgap_tty_unregister(struct board_t *brd)
> {
>         tty_unregister_driver(brd->print_driver);
>         tty_unregister_driver(brd->serial_driver);
>         put_tty_driver(brd->print_driver);
>         put_tty_driver(brd->serial_driver);
> }
>
> Very simple and nice.
>
> If you have one label it makes the code too complicated and it causes
> bugs.  We call them "one err bugs" because there is one error label.
>
> In your patch it has:
> +       dgap_tty_uninit(brd, false);
>
> But it should only be "false" if dgap_tty_init() failed.  If
> dgap_tty_register_ports() fails then it should be "true".  Another
Yes, you're right. There were no error handle for tty_port_register_device() and
dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
It need to add error handlers for them, right?

> problem is that as you say, the earlier function are allocating
> resources like dgap_tty_register() but only the last two function calls
> have a "goto err_cleanup;" so the error handling is incomplete.
So remove "goto" in dgap_firmware_load() and add error handler in
dgap_tty_init() and dgap_tty_register_ports(), right?

I have a question of this. In case of this, how to complete the error handling?
I want to cleanup allocated resources from dgap_tty_register() when
dgap_tty_init() is failed.
(and also in case of failure of dgap_tty_register_ports())
I think it can be handled as your previous e-mail.
It can have a label name of "free_chan" in dgap_tty_init() but it also have
a cleanup for allocated resource from earlier functions like
dgap_tty_register().

Can I call some functions for cleanup allocated resource from the
earlier function in dgap_tty_init() with goto label?

>
> To be honest, I think once dgap the code is cleaned up this error
> handling will be easy to write.  We shouldn't have things like:
> brd->dgap_major_serial_registered = TRUE; because we will know which
> order things were registered and unregister them in the reverse order.
OK. I will look at the code with your comment.

Really thanks for kind explanation.
regards,
Daeseok Youn
>
> regards,
> dan carpenter
>