Re: [PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer

[Viktor Rosendahl <Viktor.Rosendahl@nokia.com>]

> 
> > +		req.func_cb	= NULL;
> 
> maybe below is a better patch:
> 
> diff --git a/drivers/i2c/chips/twl4030-madc.c
> b/drivers/i2c/chips/twl4030-madc.c
> index 72b126b..6d8915e 100644
> --- a/drivers/i2c/chips/twl4030-madc.c
> +++ b/drivers/i2c/chips/twl4030-madc.c
> @@ -360,7 +360,7 @@ static int twl4030_madc_ioctl(struct inode *inode,
> struct file *filp,
>  
>         switch (cmd) {
>         case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> -               struct twl4030_madc_request req;
> +               static struct twl4030_madc_request req;
>                 if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
>                         return -EINVAL;

I don't like this idea because:

- It's fragile. This struct, which is not a const, gets initialized only
once but we are still passing a pointer to it, expecting that a fairly
complex function will not modify it. This assertion is probably true
today but makes it easier for somebody to create a bug in the future.

- You introduce another shared datum and it is only protected by the BKL
in fs/ioctl.c:vfs_ioctl().

- I didn't see any argument why this variable should be static. Making
local variables static just to get cheap zero initialization is a weird
thing to do IMO.

best regards,

Viktor