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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 28, 2013 at 4:30 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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.

I'll fix this when I work on checkpatch warnings.

> 2) Unneeded casting.

I'll send a patch for this soon.

> 3) Use sizeof(*brd) instead of sizeof(struct board_t).

Okay.

> 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.

Okay, I'll change it to dgnc_board.

> 5) Put the two assignments on two lines.  First assign "brd" and then
>    initialize "dgnc_Board[dgnc_NumBoards]" after allocating
>    "brd->msgbuf".

Okay.

> 6) Comment is obvious and at the same time wrong.  It means "allocate
>    the board structure" instead of "get".  Delete.

Okay.

> 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.

Okay, I'll remove that.

> 8) No parenthesis around the return.

Okay, I'll fix this when I work on checkpatch warnings.

>> @@ -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);

Okay.

> 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...

Okay, I'll replace error handling with goto statements
where it is appropriate.

Thanks for the feedback.

I'm going to be making a TODO for this driver. If there's
anything else I can do to fix it, let me know.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux