On Mon, Sep 23, 2013 at 8:27 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Mon, Sep 23, 2013 at 06:47:16PM -0400, Lidza Louina wrote:
>> This patch removes these warnings:
>> potential null dereference 'brd->SerialDriver'. (alloc_tty_driver returns null)
>> potential null dereference 'brd->PrintDriver'. (alloc_tty_driver returns null)
>>
>> This warning popped up because there wasn't a check
>> to make sure that the serial and print drivers were
>> allocated and not null before being initialized. This
>> patch adds that check.
>>
>> Signed-off-by: Lidza Louina <lidza.louina@xxxxxxxxx>
>> ---
>> drivers/staging/dgap/dgap_tty.c | 103 +++++++++++++++++++++-------------------
>> 1 file changed, 54 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
>> index 924e2bf..8f0a824 100644
>> --- a/drivers/staging/dgap/dgap_tty.c
>> +++ b/drivers/staging/dgap/dgap_tty.c
>> @@ -220,66 +220,71 @@ int dgap_tty_register(struct board_t *brd)
>> DPR_INIT(("tty_register start"));
>>
>> brd->SerialDriver = alloc_tty_driver(MAXPORTS);
>> + if(brd->SerialDriver){
>
> Don't do it that way, flip it around.
>
> brd->SerialDriver = alloc_tty_driver(MAXPORTS);
> if (!brd->SerialDriver)
> return -ENOMEM;
>
> snprintf(brd->SerialName, MAXTTYNAMELEN, "tty_dgap_%d_", brd->boardnum);
> brd->SerialDriver->name = brd->SerialName;
>
> That way has fewer indents.
>
> When you're writing code, you want it to read in a straight line going
> down. You don't want long if statement blocks or spaghetti code. If
> you hit an error deal with it immediately and continue with the story in
> a straight line going down.
>
>> +
>> + /* The kernel wants space to store pointers to tty_structs */
>> + brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
>> + if (!brd->SerialDriver->ttys)
>> + return(-ENOMEM);
>
> On this if statement it's a little bit more complicated because you have
> to free the memory you allocated earlier before returning. This one
> should look like:
>
> brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
> if (!brd->SerialDriver->ttys) {
> ret = -ENOMEM;
> goto err_put_driver;
> }
> tty_set_operations(brd->SerialDriver, &dgap_tty_ops);
>
>
> At the end of the function there will be something like:
>
> return 0;
>
> err_release_foo:
> release_foo();
> err_free_something:
> free_some_more_stuff();
> err_put_driver:
> put_tty_driver(brd->SerialDriver);
>
> return ret;
> }
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? >_<
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
