[PATCH v2 2/3] staging: dgnc: driver.c and tty.c: replaces dgnc_driver_kzmalloc with kzalloc

[Lidza Louina]

On Wed, Aug 28, 2013 at 4:30 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> On Tue, Aug 27, 2013 at 10:13:27PM -0400, Lidza Louina wrote:
>> @@ -501,7 +501,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
>>
>>       /* get the board structure and prep it */
>>       brd = dgnc_Board[dgnc_NumBoards] =
>> -     (struct board_t *) dgnc_driver_kzmalloc(sizeof(struct board_t), GFP_KERNEL);
>> +     (struct board_t *) kzalloc(sizeof(struct board_t), GFP_KERNEL);
>>       if (!brd) {
>>               APR(("memory allocation for board structure failed\n"));
>>               return(-ENOMEM);
>
> So you didn't introduce this, but here are the style problems in this
> section, in case you want to fix them in a later patch.
>
> 1) Bad indenting.
> 2) Unneeded casting.
> 3) Use sizeof(*brd) instead of sizeof(struct board_t).
> 4) board_t is a bad and wrong name for this data type.  "board" is too
>    generic, and "_t" means "typedef" but this is a not a typedef.
> 5) Put the two assignments on two lines.  First assign "brd" and then
>    initialize "dgnc_Board[dgnc_NumBoards]" after allocating
>    "brd->msgbuf".
> 6) Comment is obvious and at the same time wrong.  It means "allocate
>    the board structure" instead of "get".  Delete.
> 7) kmalloc() has its own error message which is much more useful.  No
>    need to print a useless error message.  Also this error will never
>    occur in real life so adding code for something which will never
>    happen and it's a waste of time for people reading the code.
> 8) No parenthesis around the return.
>
>> @@ -509,7 +509,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
>>
>>       /* make a temporary message buffer for the boot messages */
>>       brd->msgbuf = brd->msgbuf_head =
>> -             (char *) dgnc_driver_kzmalloc(sizeof(char) * 8192, GFP_KERNEL);
>> +             (char *) kzalloc(sizeof(char) * 8192, GFP_KERNEL);
>>       if (!brd->msgbuf) {
>>               kfree(brd);
>>               APR(("memory allocation for board msgbuf failed\n"));
>
> 9)  I think we know the sizeof(char)...  If we want to keep that then
>     use kcalloc() instead of kzalloc().  But it's simplest to just say:
>
>     brd->msgbuf = kzalloc(8192, GFP_KERNEL);
>
> 10) The error handling should be:
>
>         if (!brd->msgbuf) {
>                 ret = -ENOMEM;
>                 goto err_free_brd;
>         }
>
>         [snip code in the middle]
>
>         return 0;
>
> err_free_brd:
>         kfree(brd);
>
>         return ret;
>
>     We leak memory later on in this function...

Also, can I put your name in the Reported-by: section of these patches?