Hi, Dan
Please look at below my line comment.
2014-05-28 16:02 GMT+09:00 Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> 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
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
