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
