[PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

Lidza Louina lidza.louina at gmail.com
Tue Sep 24 18:40:10 UTC 2013


On Mon, Sep 23, 2013 at 8:27 PM, Dan Carpenter <dan.carpenter at oracle.com> 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 at gmail.com>
>> ---
>>  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? >_<


More information about the devel mailing list