[PATCH] PM/CORE: Fix using sizeof without parenthesis in the sysfs.c

[PATCH] PM/CORE: Fix using sizeof without parenthesis in the sysfs.c

[Lan Tianyu]

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(-)

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;
-- 
1.7.10.4

Re: [PATCH] PM/CORE: Fix using sizeof without parenthesis in the sysfs.c

[James Courtier-Dutton]

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

Re: [PATCH] PM/CORE: Fix using sizeof without parenthesis in the sysfs.c

[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)

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM/CORE: Fix using sizeof without parenthesis in the sysfs.c

[Lan Tianyu]

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

-- 
Best regards
Tianyu Lan
--
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

Re: [PATCH] PM/CORE: Fix using sizeof without parenthesis in the sysfs.c

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

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM/CORE: Fix using sizeof without parenthesis in the sysfs.c

[Lan Tianyu]

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

-- 
Best regards
Tianyu Lan