Re: [PATCH v2 6/6] staging: dgnc: changes arguments in sizeof

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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];
};

int main(void)
{
	struct foo *p;

	printf("%ld %ld\n", sizeof(p), sizeof(*p));

	return 0;
}

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

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel