On Tue, Sep 24, 2013 at 3:20 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 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.
Okay, I read that. Thanks.
> 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;
I looked at other uses of the function alloc_tty_driver() in
the kernel and none of them seem to follow up with a
call to kfree(). Are they supposed to? I realize that
because the allocation failed, I wouldn't need to call
kfree afterward. >_< So this should be enough:
if(!brd->PrintDriver){
rc = -ENOMEM;
}
The code then returns rc at the end of the function.
Is this code supposed to include a call to kfree? If so,
what makes this driver different from the others?
> 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.
Okay, thanks for the tip. I'll keep that in mind
whenever I use goto statements. :)
> 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
Hmm, ok, I'll do that next on this driver. >_< Add
dgap_tty_unregister() and make sure it and
dgap_tty_register() get called.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
