On Tue, Sep 24, 2013 at 02:40:10PM -0400, Lidza Louina wrote:
> Instead of writing:
> brd->SerialDriver = alloc_tty_driver(MAXPORTS);
> if (!brd->SerialDriver){
> goto free_stuff;
> return -ENOMEM;
> }
>
> Would it correct if I wrote:
> brd->SerialDriver = alloc_tty_driver(MAXPORTS);
> if (!brd->SerialDriver){
> kfree(brd->SerialDriver);
> return -ENOMEM;
> }
>
> Just to avoid writing goto statements? >_<
No. The problem with doing it that way is that we end up with multiple
calls to kfree(brd->SerialDriver);. The error handling becomes more
complex and error prone.
This is explained in Documentation/CodingStyle.
It should be written in mirror format. All the resources are freed in
the reverse order that they are allocated.
one = kmalloc();
if (!one)
return -ENOMEM;
two = kmalloc();
if (!two) {
ret = -ENOMEM;
goto err_free_one;
}
three = kmalloc();
if (!three) {
ret = -ENOMEM;
goto err_free_two;
}
ret = frob();
if (ret)
goto err_free_three;
err_free_three:
kfree(three);
err_free_two:
kfree(two);
err_free_one:
kfree(one);
return ret;
Most of the time, there shouldn't be any if statements in the cleanup.
A common mistake is to use GW-BASIC names.
err_1:
kfree(foo);
err_2:
kfree(bar);
That's not useful because it doesn't describe what happens at the label.
Another mistake is to choose label names based on the goto location.
if (kmalloc_failed)
goto err_kmalloc_failed;
I already know kmalloc failed so the label name is totally useless.
Once you have your error handling cleanup block at the end of the
function, then that should almost match the release() function. Btw, I
notice this dgap_tty_register() function doesn't have a matching
unregister function and actually dgap_tty_register() is never called.
:P
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
