[PATCH v2 4/5] staging: dgap: tty.c: changes error handing to tty driver allocations

Lidza Louina lidza.louina at gmail.com
Thu Sep 26 22:48:04 UTC 2013


On Thu, Sep 26, 2013 at 6:19 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> This one is not right.
>
> On Wed, Sep 25, 2013 at 07:08:53PM -0400, Lidza Louina wrote:
>> This patch changes error handling to the
>> tty_driver allocations in dgap_tty_register.
>>
>> Before, it didn't handle the possibility of an
>> alloc_tty_driver failure. This patch makes
>> dgap_register_driver return -ENOMEM if that fails.
>>
>> This patch also adds handling to the possibility that
>> the brd->SerialDriver->ttys or brd->PrintDriver->ttys
>> allocation will fail. It now calls put_tty_driver on
>> that driver after it fails and returns -ENOMEM.
>>
>> Signed-off-by: Lidza Louina <lidza.louina at gmail.com>
>> ---
>>  drivers/staging/dgap/dgap_tty.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
>> index 59fda2e..3e12ddb 100644
>> --- a/drivers/staging/dgap/dgap_tty.c
>> +++ b/drivers/staging/dgap/dgap_tty.c
>> @@ -220,6 +220,8 @@ int dgap_tty_register(struct board_t *brd)
>>       DPR_INIT(("tty_register start"));
>>
>>       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;
>> @@ -234,9 +236,10 @@ int dgap_tty_register(struct board_t *brd)
>>
>>       /* 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);
>> -
>> +     if (!brd->SerialDriver->ttys){
>> +             rc = -ENOMEM;
>> +             goto err_put_tty_serial;
>> +     }
>>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
>>       brd->SerialDriver->refcount = brd->TtyRefCnt;
>>  #endif
>> @@ -253,6 +256,8 @@ int dgap_tty_register(struct board_t *brd)
>>        * we are when we get into the dgap_tty_open() routine.
>>        */
>>       brd->PrintDriver = alloc_tty_driver(MAXPORTS);
>> +     if (!brd->PrintDriver)
>> +             return -ENOMEM;
>
>         if (!brd->PrintDriver) {
>                 rc = -ENOMEM:
>                 goto err_free_serial_ttys;
>         }
>
>>
>>       snprintf(brd->PrintName, MAXTTYNAMELEN, "pr_dgap_%d_", brd->boardnum);
>>       brd->PrintDriver->name = brd->PrintName;
>> @@ -267,9 +272,10 @@ int dgap_tty_register(struct board_t *brd)
>>
>>       /* The kernel wants space to store pointers to tty_structs */
>>       brd->PrintDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
>> -     if (!brd->PrintDriver->ttys)
>> -             return(-ENOMEM);
>> -
>> +     if (!brd->PrintDriver->ttys){
>> +             rc = -ENOMEM;
>> +             goto err_put_tty_print;
>> +     }
>>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
>>       brd->PrintDriver->refcount = brd->TtyRefCnt;
>>  #endif
>> @@ -285,7 +291,7 @@ int dgap_tty_register(struct board_t *brd)
>>               rc = tty_register_driver(brd->SerialDriver);
>>               if (rc < 0) {
>>                       APR(("Can't register tty device (%d)\n", rc));
>> -                     return(rc);
>> +                     goto err_put_tty_serial;
>
>                         goto err_free_print_ttys;
>
>>               }
>>               brd->dgap_Major_Serial_Registered = TRUE;
>>               dgap_BoardsByMajor[brd->SerialDriver->major] = brd;
>> @@ -297,7 +303,7 @@ int dgap_tty_register(struct board_t *brd)
>>               rc = tty_register_driver(brd->PrintDriver);
>>               if (rc < 0) {
>>                       APR(("Can't register Transparent Print device (%d)\n", rc));
>> -                     return(rc);
>> +                     goto err_put_tty_print;
>
>                         goto err_unregister_serial;
>
>>               }
>>               brd->dgap_Major_TransparentPrint_Registered = TRUE;
>>               dgap_BoardsByMajor[brd->PrintDriver->major] = brd;
>> @@ -307,6 +313,10 @@ int dgap_tty_register(struct board_t *brd)
>>       DPR_INIT(("DGAP REGISTER TTY: MAJORS: %d %d\n", brd->SerialDriver->major,
>>               brd->PrintDriver->major));
>
> Your patch frees everything by mistake on the success path.  It should
> be:
>
>         return 0;
>
> err_unregister_serial:
>         tty_unregister_driver(brd->SerialDriver);
> err_free_print_ttys:
>         kfree(brd->PrintDriver->ttys);
> err_put_tty_print:
>         put_tty_driver(brd->PrintDriver);
> err_free_serial_ttys:
>         kfree(brd->SerialDriver->ttys);
> err_put_tty_serial:
>         put_tty_driver(brd->SerialDriver);
>
>         return rc;
> }
>
> regards,
> dan carpenter

Okay, I'll resend this patch. Thanks.


More information about the devel mailing list