| Message ID | 1359361909-28793-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On 28 January 2013 08:31, Lan Tianyu <tianyu.lan@intel.com> wrote: > - if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0) > + if (len == sizeof(ctrl_auto - 1) && strncmp(buf, ctrl_auto, len) == 0) This looks wrong to me. sizeof ctrl_auto - 1 is not the same value as sizeof(ctrl_auto - 1) because sizeof(x) is normally the same as sizeof(x - 1), unless sizeof x and sizeof 1 are different. Consider that is maybe should be: if (len == (sizeof(ctrl_auto) - 1)) && strncmp(buf, ctrl_auto, len) == 0) I a not sure what the correct answer is for this particular bit of code, because I have not looked at it in detail,I just wanted to point out that the brackets might be in the wrong place here. James -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, January 28, 2013 09:03:28 AM James Courtier-Dutton wrote: > On 28 January 2013 08:31, Lan Tianyu <tianyu.lan@intel.com> wrote: > > - if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0) > > + if (len == sizeof(ctrl_auto - 1) && strncmp(buf, ctrl_auto, len) == 0) > > This looks wrong to me. > sizeof ctrl_auto - 1 > is not the same value as > sizeof(ctrl_auto - 1) > because sizeof(x) is normally the same as sizeof(x - 1), unless sizeof > x and sizeof 1 are different. > Consider that is maybe should be: > if (len == (sizeof(ctrl_auto) - 1)) && strncmp(buf, ctrl_auto, len) == 0) The outer parentheses in the comparison with len are not necessary. > I a not sure what the correct answer is for this particular bit of > code, because I have not looked at it in detail,I just wanted to point > out that the brackets might be in the wrong place here. You are right and the patch doesn't make sense. Thanks, Rafael
? 2013/1/28 20:18, Rafael J. Wysocki ??: > On Monday, January 28, 2013 09:03:28 AM James Courtier-Dutton wrote: >> On 28 January 2013 08:31, Lan Tianyu <tianyu.lan@intel.com> wrote: >>> - if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0) >>> + if (len == sizeof(ctrl_auto - 1) && strncmp(buf, ctrl_auto, len) == 0) >> >> This looks wrong to me. >> sizeof ctrl_auto - 1 >> is not the same value as >> sizeof(ctrl_auto - 1) >> because sizeof(x) is normally the same as sizeof(x - 1), unless sizeof >> x and sizeof 1 are different. >> Consider that is maybe should be: >> if (len == (sizeof(ctrl_auto) - 1)) && strncmp(buf, ctrl_auto, len) == 0) Hi James: Yes. You are correct. Thanks for your review. > > The outer parentheses in the comparison with len are not necessary. > >> I a not sure what the correct answer is for this particular bit of >> code, because I have not looked at it in detail,I just wanted to point >> out that the brackets might be in the wrong place here. > > You are right and the patch doesn't make sense. Hi Rafael: So this patch is not necessary? > > Thanks, > Rafael > >
On Monday, January 28, 2013 09:04:29 PM Lan Tianyu wrote: > ? 2013/1/28 20:18, Rafael J. Wysocki ??: > > On Monday, January 28, 2013 09:03:28 AM James Courtier-Dutton wrote: > >> On 28 January 2013 08:31, Lan Tianyu <tianyu.lan@intel.com> wrote: > >>> - if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0) > >>> + if (len == sizeof(ctrl_auto - 1) && strncmp(buf, ctrl_auto, len) == 0) > >> > >> This looks wrong to me. > >> sizeof ctrl_auto - 1 > >> is not the same value as > >> sizeof(ctrl_auto - 1) > >> because sizeof(x) is normally the same as sizeof(x - 1), unless sizeof > >> x and sizeof 1 are different. > >> Consider that is maybe should be: > >> if (len == (sizeof(ctrl_auto) - 1)) && strncmp(buf, ctrl_auto, len) == 0) > Hi James: > Yes. You are correct. Thanks for your review. > > > > The outer parentheses in the comparison with len are not necessary. > > > >> I a not sure what the correct answer is for this particular bit of > >> code, because I have not looked at it in detail,I just wanted to point > >> out that the brackets might be in the wrong place here. > > > > You are right and the patch doesn't make sense. > Hi Rafael: > So this patch is not necessary? It is incorrect in the first place. Yes, you could change spaces to parentheses in those places, but first, please do that correctly and second, it's just going to be a cosmetic change. The code works as is. Thanks, Rafael
? 2013/1/28 21:13, Rafael J. Wysocki ??: > On Monday, January 28, 2013 09:04:29 PM Lan Tianyu wrote: >> ? 2013/1/28 20:18, Rafael J. Wysocki ??: >>> On Monday, January 28, 2013 09:03:28 AM James Courtier-Dutton wrote: >>>> On 28 January 2013 08:31, Lan Tianyu <tianyu.lan@intel.com> wrote: >>>>> - if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0) >>>>> + if (len == sizeof(ctrl_auto - 1) && strncmp(buf, ctrl_auto, len) == 0) >>>> >>>> This looks wrong to me. >>>> sizeof ctrl_auto - 1 >>>> is not the same value as >>>> sizeof(ctrl_auto - 1) >>>> because sizeof(x) is normally the same as sizeof(x - 1), unless sizeof >>>> x and sizeof 1 are different. >>>> Consider that is maybe should be: >>>> if (len == (sizeof(ctrl_auto) - 1)) && strncmp(buf, ctrl_auto, len) == 0) >> Hi James: >> Yes. You are correct. Thanks for your review. >>> >>> The outer parentheses in the comparison with len are not necessary. >>> >>>> I a not sure what the correct answer is for this particular bit of >>>> code, because I have not looked at it in detail,I just wanted to point >>>> out that the brackets might be in the wrong place here. >>> >>> You are right and the patch doesn't make sense. >> Hi Rafael: >> So this patch is not necessary? > > It is incorrect in the first place. > Yeah. Sorry for this and will be careful. > Yes, you could change spaces to parentheses in those places, but first, please > do that correctly and second, it's just going to be a cosmetic change. The > code works as is. Ok. I get it. > > Thanks, > Rafael > >
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c index 50d16e3..f089b80 100644 --- a/drivers/base/power/sysfs.c +++ b/drivers/base/power/sysfs.c @@ -119,9 +119,9 @@ static ssize_t control_store(struct device * dev, struct device_attribute *attr, if (cp) len = cp - buf; device_lock(dev); - if (len == sizeof ctrl_auto - 1 && strncmp(buf, ctrl_auto, len) == 0) + if (len == sizeof(ctrl_auto - 1) && strncmp(buf, ctrl_auto, len) == 0) pm_runtime_allow(dev); - else if (len == sizeof ctrl_on - 1 && strncmp(buf, ctrl_on, len) == 0) + else if (len == sizeof(ctrl_on - 1) && strncmp(buf, ctrl_on, len) == 0) pm_runtime_forbid(dev); else n = -EINVAL; @@ -321,11 +321,11 @@ wake_store(struct device * dev, struct device_attribute *attr, cp = memchr(buf, '\n', n); if (cp) len = cp - buf; - if (len == sizeof enabled - 1 - && strncmp(buf, enabled, sizeof enabled - 1) == 0) + if (len == sizeof(enabled - 1) + && strncmp(buf, enabled, sizeof(enabled - 1)) == 0) device_set_wakeup_enable(dev, 1); - else if (len == sizeof disabled - 1 - && strncmp(buf, disabled, sizeof disabled - 1) == 0) + else if (len == sizeof(disabled - 1) + && strncmp(buf, disabled, sizeof(disabled - 1)) == 0) device_set_wakeup_enable(dev, 0); else return -EINVAL; @@ -546,9 +546,9 @@ static ssize_t async_store(struct device *dev, struct device_attribute *attr, cp = memchr(buf, '\n', n); if (cp) len = cp - buf; - if (len == sizeof enabled - 1 && strncmp(buf, enabled, len) == 0) + if (len == sizeof(enabled - 1) && strncmp(buf, enabled, len) == 0) device_enable_async_suspend(dev); - else if (len == sizeof disabled - 1 && strncmp(buf, disabled, len) == 0) + else if (len == sizeof(disabled - 1) && strncmp(buf, disabled, len) == 0) device_disable_async_suspend(dev); else return -EINVAL;
Kernel style uses parenthesis around sizeof. Commit 66c80b60 has already added such check in the scripts/checkpatch.pl. Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- drivers/base/power/sysfs.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)