On Fri, Sep 6, 2013 at 5:45 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Fri, Sep 06, 2013 at 04:48:32PM -0400, Lidza Louina wrote:
>> The arguments that are passed into sizeof were
>> generic. This patch changes this by putting
>> the actual item that we need a size of instead.
>>
>> For example:
>> - kzalloc(sizeof(struct dgnc_board), GFP_KERNEL);
>> + kzalloc(sizeof(brd), GFP_KERNEL);
>>
>> Signed-off-by: Lidza Louina <lidza.louina@xxxxxxxxx>
>> ---
>> drivers/staging/dgnc/dgnc_driver.c | 4 ++--
>> drivers/staging/dgnc/dgnc_mgmt.c | 2 +-
>> drivers/staging/dgnc/dgnc_tty.c | 24 ++++++++++++------------
>> 3 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/staging/dgnc/dgnc_driver.c b/drivers/staging/dgnc/dgnc_driver.c
>> index 5b4d799..a1b24b5 100644
>> --- a/drivers/staging/dgnc/dgnc_driver.c
>> +++ b/drivers/staging/dgnc/dgnc_driver.c
>> @@ -487,14 +487,14 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
>>
>> /* get the board structure and prep it */
>> brd = dgnc_Board[dgnc_NumBoards] =
>> - kzalloc(sizeof(struct dgnc_board), GFP_KERNEL);
>> + kzalloc(sizeof(brd), GFP_KERNEL);
>
> Still not right.
>
> sizeof(*brd);
>
> I'm always creating test small test programs:
>
> struct foo {
> char buf[42];> regards,
> dan carpenter
>
> };
>
> int main(void)
> {
> struct foo *p;
>
> printf("%ld %ld\n", sizeof(p), sizeof(*p));
>
> return 0;
> }
Okay, I see that the sizeof the *p is larger than
the sizeof p.
>> if (!brd) {
>> return -ENOMEM;
>> }
>>
>> /* make a temporary message buffer for the boot messages */
>> brd->msgbuf = brd->msgbuf_head =
>> - kzalloc(sizeof(char) * 8192, GFP_KERNEL);
>> + kzalloc(sizeof(u8) * 8192, GFP_KERNEL);
>> if (!brd->msgbuf) {
>> kfree(brd);
>> return -ENOMEM;
>> diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c
>> index bb39f5d..354458c 100644
>> --- a/drivers/staging/dgnc/dgnc_mgmt.c
>> +++ b/drivers/staging/dgnc/dgnc_mgmt.c
>> @@ -209,7 +209,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> uint board = 0;
>> uint channel = 0;
>>
>> - if (copy_from_user(&ni, uarg, sizeof(struct ni_info))) {
>> + if (copy_from_user(&ni, uarg, sizeof(ni))) {
>> return -EFAULT;
>> }
>>
>> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
>> index fe38529..894b7df 100644
>> --- a/drivers/staging/dgnc/dgnc_tty.c
>> +++ b/drivers/staging/dgnc/dgnc_tty.c
>> @@ -200,8 +200,8 @@ int dgnc_tty_register(struct dgnc_board *brd)
>>
>> DPR_INIT(("tty_register start\n"));
>>
>> - memset(&brd->SerialDriver, 0, sizeof(struct tty_driver));
>> - memset(&brd->PrintDriver, 0, sizeof(struct tty_driver));
>> + memset(&brd->SerialDriver, 0, sizeof(brd->SerialDriver));
>> + memset(&brd->PrintDriver, 0, sizeof(brd->PrintDriver));
>>
>> brd->SerialDriver.magic = TTY_DRIVER_MAGIC;
>>
>> @@ -222,12 +222,12 @@ int dgnc_tty_register(struct dgnc_board *brd)
>> * The kernel wants space to store pointers to
>> * tty_struct's and termios's.
>> */
>> - brd->SerialDriver.ttys = kzalloc(brd->maxports * sizeof(struct tty_struct *), GFP_KERNEL);
>> + brd->SerialDriver.ttys = kzalloc(brd->maxports * sizeof(brd->SerialDriver.ttys), GFP_KERNEL);
>
> ttys is a pointer to a pointer. What you have works but it would be
> better to say. sizeof(*brd->SerialDriver.ttys). For this one kcalloc()
> is actually better than kzalloc(). It's cleaner and it has overflow
> protection built in.
>
>
>> if (!brd->SerialDriver.ttys)
>> return -ENOMEM;
>>
>> kref_init(&brd->SerialDriver.kref);
>> - brd->SerialDriver.termios = kzalloc(brd->maxports * sizeof(struct ktermios *), GFP_KERNEL);
>> + brd->SerialDriver.termios = kzalloc(brd->maxports * sizeof(brd->SerialDriver.termios), GFP_KERNEL);
>
> Same.
>
>> if (!brd->SerialDriver.termios)
>> return -ENOMEM;
>>
>> @@ -271,11 +271,11 @@ int dgnc_tty_register(struct dgnc_board *brd)
>> * tty_struct's and termios's. Must be seperate from
>> * the Serial Driver so we don't get confused
>> */
>> - brd->PrintDriver.ttys = kzalloc(brd->maxports * sizeof(struct tty_struct *), GFP_KERNEL);
>> + brd->PrintDriver.ttys = kzalloc(brd->maxports * sizeof(brd->PrintDriver.ttys), GFP_KERNEL);
>
> Same.
>
>> if (!brd->PrintDriver.ttys)
>> return -ENOMEM;
>> kref_init(&brd->PrintDriver.kref);
>> - brd->PrintDriver.termios = kzalloc(brd->maxports * sizeof(struct ktermios *), GFP_KERNEL);
>> + brd->PrintDriver.termios = kzalloc(brd->maxports * sizeof(brd->PrintDriver.termios), GFP_KERNEL);
>
> Same.
>
>
>> if (!brd->PrintDriver.termios)
>> return -ENOMEM;
>>
>> @@ -341,7 +341,7 @@ int dgnc_tty_init(struct dgnc_board *brd)
>> * Okay to malloc with GFP_KERNEL, we are not at
>> * interrupt context, and there are no locks held.
>> */
>> - brd->channels[i] = kzalloc(sizeof(struct channel_t), GFP_KERNEL);
>> + brd->channels[i] = kzalloc(sizeof(brd->channels[i]), GFP_KERNEL);
>
> This one is buggy. It should be:
>
> brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
>
> But really that's awkward so the original is probably fine.
I thought that if it were sizeof(struct dgnc_board)
then I would need to replace it with a board itself and
not a pointer to it.
I'm going to replace them with pointers to specific
instances of the structs. And I'll use kcalloc for arrays
(i.e brd->PrintDriver.termios and brd->PrintDriver.ttys).
Thanks,
Lidza
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
