Re: [PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer
* Viktor Rosendahl [EMAIL PROTECTED] [080703 18:37]:
+ 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.
Pushing the original patch.
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer
+ 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] twl4030-madc: Fix arbitrarily initialized function pointer
req is an automatic variable and thus we cannot rely on it being initialized to
zero (I am leaving the 0!= NULL discussion aside). Other functions test
if this pointer is NULL, in order to determine whether it is a valid address or
not.
Signed-off-by: Viktor Rosendahl [EMAIL PROTECTED]
---
drivers/i2c/chips/twl4030-madc.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/chips/twl4030-madc.c b/drivers/i2c/chips/twl4030-madc.c
index 72b126b..743db74 100644
--- a/drivers/i2c/chips/twl4030-madc.c
+++ b/drivers/i2c/chips/twl4030-madc.c
@@ -367,6 +367,7 @@ static int twl4030_madc_ioctl(struct inode *inode, struct
file *filp,
req.channels = (1par.channel);
req.do_avg = par.average;
req.method = TWL4030_MADC_SW1;
+ req.func_cb = NULL;
val = twl4030_madc_conversion(req);
if (val = 0) {
--
1.5.5.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
