[lm-sensors] [PATCH] hwmon: Add missing parentheses

[lm-sensors] [PATCH] hwmon: Add missing parentheses

[Roel Kluin]

I think this was intended? please review.
--------------------------->8-------------8<------------------------------
Add missing parentheses

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/hwmon/abituguru3.c b/drivers/hwmon/abituguru3.c
index e52b388..fd98685 100644
--- a/drivers/hwmon/abituguru3.c
+++ b/drivers/hwmon/abituguru3.c
@@ -761,7 +761,7 @@ static int abituguru3_read_increment_offset(struct abituguru3_data *data,
 	for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
 				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+			return i * count + ((i && (x < 0)) ? 0 : x);
 
 	return i * count;
 }

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[Alistair John Strachan]

On Tuesday 17 February 2009 13:17:54 Roel Kluin wrote:
> I think this was intended? please review.

This one's been there from the beginning.

The change looks right to me. (i * count) account the previous successes, add 
either zero or x depending on the kind of read failure (timeout/short read vs 
other I/O error). I'm not entirely sure why i must be >0 for accounting the 
short read.

Nothing calling abituguru3_read_increment_offset() checks for anything other 
than complete success. Every caller passes count=1, and offset_count>1, so 
both the new code and the old could never return a full count 
(offset_count*count) when an error has occured (old returns either 0 or x, 
x=count). Ergo, the change can't cause any regressions.

Acked-by: Alistair John Strachan <alistair@devzero.co.uk>

Jean, are you happy to take this?

> --------------------------->8-------------8<------------------------------
> Add missing parentheses
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/hwmon/abituguru3.c b/drivers/hwmon/abituguru3.c
> index e52b388..fd98685 100644
> --- a/drivers/hwmon/abituguru3.c
> +++ b/drivers/hwmon/abituguru3.c
> @@ -761,7 +761,7 @@ static int abituguru3_read_increment_offset(struct
> abituguru3_data *data, for (i = 0; i < offset_count; i++)
>  		if ((x = abituguru3_read(data, bank, offset + i, count,
>  				buf + i * count)) != count)
> -			return i * count + (i && (x < 0)) ? 0 : x;
> +			return i * count + ((i && (x < 0)) ? 0 : x);
>
>  	return i * count;
>  }

-- 
Cheers,
Alistair.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[Jean Delvare]

On Tue, 17 Feb 2009 21:03:23 +0000, Alistair John Strachan wrote:
> On Tuesday 17 February 2009 13:17:54 Roel Kluin wrote:
> > I think this was intended? please review.
> 
> This one's been there from the beginning.
> 
> The change looks right to me. (i * count) account the previous successes, add 
> either zero or x depending on the kind of read failure (timeout/short read vs 
> other I/O error). I'm not entirely sure why i must be >0 for accounting the 
> short read.

I can't make any sense of that "i &&" either. Hans?

> Nothing calling abituguru3_read_increment_offset() checks for anything other 
> than complete success. Every caller passes count=1, and offset_count>1, so 
> both the new code and the old could never return a full count 
> (offset_count*count) when an error has occured (old returns either 0 or x, 
> x=count). Ergo, the change can't cause any regressions.
> 
> Acked-by: Alistair John Strachan <alistair@devzero.co.uk>
> 
> Jean, are you happy to take this?

Yes, but I'd like to understand why the "i &&" is there, and if there's
no good reason we should drop it.

> > --------------------------->8-------------8<------------------------------
> > Add missing parentheses
> >
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > diff --git a/drivers/hwmon/abituguru3.c b/drivers/hwmon/abituguru3.c
> > index e52b388..fd98685 100644
> > --- a/drivers/hwmon/abituguru3.c
> > +++ b/drivers/hwmon/abituguru3.c
> > @@ -761,7 +761,7 @@ static int abituguru3_read_increment_offset(struct
> > abituguru3_data *data, for (i = 0; i < offset_count; i++)
> >  		if ((x = abituguru3_read(data, bank, offset + i, count,
> >  				buf + i * count)) != count)
> > -			return i * count + (i && (x < 0)) ? 0 : x;
> > +			return i * count + ((i && (x < 0)) ? 0 : x);
> >
> >  	return i * count;
> >  }
> 


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[Hans de Goede]

Jean Delvare wrote:
> On Tue, 17 Feb 2009 21:03:23 +0000, Alistair John Strachan wrote:
>> On Tuesday 17 February 2009 13:17:54 Roel Kluin wrote:
>>> I think this was intended? please review.
>> This one's been there from the beginning.
>>
>> The change looks right to me. (i * count) account the previous successes, add 
>> either zero or x depending on the kind of read failure (timeout/short read vs 
>> other I/O error). I'm not entirely sure why i must be >0 for accounting the 
>> short read.
> 
> I can't make any sense of that "i &&" either. Hans?
> 

This is in an error handling path, if some bytes were read but not count bytes, 
then we return i * count + x, where i * count is the number of bytes read 
successful so far and x is the number of bytes read for the offset we were 
reading from when we got the short read.

However if this is not just a short read, but a read which error-ed out then x 
< 0. If we then would return i * count + x, we would return less then what 
we've actually read. So in this case we want to return just i * count, which 
gets done in the form of i * count + 0.

There is one special case however if we get an error code in the form of x < 0, 
and we've not done any successfull reads (so i = 0), then the right thing to 
do is to pass the error code along. So the condition in which we want to add 0 
to the result instead of x becomes (i && x < 0)

Hopes this helps.

Regards,

Hans

>> Nothing calling abituguru3_read_increment_offset() checks for anything other 
>> than complete success. Every caller passes count=1, and offset_count>1, so 
>> both the new code and the old could never return a full count 
>> (offset_count*count) when an error has occured (old returns either 0 or x, 
>> x=count). Ergo, the change can't cause any regressions.
>>
>> Acked-by: Alistair John Strachan <alistair@devzero.co.uk>
>>
>> Jean, are you happy to take this?
> 
> Yes, but I'd like to understand why the "i &&" is there, and if there's
> no good reason we should drop it.
> 
>>> --------------------------->8-------------8<------------------------------
>>> Add missing parentheses
>>>
>>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>>> ---
>>> diff --git a/drivers/hwmon/abituguru3.c b/drivers/hwmon/abituguru3.c
>>> index e52b388..fd98685 100644
>>> --- a/drivers/hwmon/abituguru3.c
>>> +++ b/drivers/hwmon/abituguru3.c
>>> @@ -761,7 +761,7 @@ static int abituguru3_read_increment_offset(struct
>>> abituguru3_data *data, for (i = 0; i < offset_count; i++)
>>>  		if ((x = abituguru3_read(data, bank, offset + i, count,
>>>  				buf + i * count)) != count)
>>> -			return i * count + (i && (x < 0)) ? 0 : x;
>>> +			return i * count + ((i && (x < 0)) ? 0 : x);
>>>
>>>  	return i * count;
>>>  }
> 
> 

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[Jean Delvare]

Hi Hans,

On Wed, 18 Feb 2009 15:25:52 +0100, Hans de Goede wrote:
> Jean Delvare wrote:
> > On Tue, 17 Feb 2009 21:03:23 +0000, Alistair John Strachan wrote:
> >> This one's been there from the beginning.
> >>
> >> The change looks right to me. (i * count) account the previous successes, add 
> >> either zero or x depending on the kind of read failure (timeout/short read vs 
> >> other I/O error). I'm not entirely sure why i must be >0 for accounting the 
> >> short read.
> > 
> > I can't make any sense of that "i &&" either. Hans?
> 
> This is in an error handling path, if some bytes were read but not count bytes, 
> then we return i * count + x, where i * count is the number of bytes read 
> successful so far and x is the number of bytes read for the offset we were 
> reading from when we got the short read.
> 
> However if this is not just a short read, but a read which error-ed out then x 
> < 0. If we then would return i * count + x, we would return less then what 
> we've actually read. So in this case we want to return just i * count, which 
> gets done in the form of i * count + 0.

So far so good...

> There is one special case however if we get an error code in the form of x < 0, 
> and we've not done any successfull reads (so i = 0), then the right thing to 
> do is to pass the error code along. So the condition in which we want to add 0 
> to the result instead of x becomes (i && x < 0)
> 
> Hopes this helps.

OK... I get the idea now. I'm glad you were still around to explain it
because I would never have figured it out. And I don't really like the
logic: behaving differently if the error occurs during the first read
or the later ones doesn't make a lot of sense to me. If the error is so
important that it needs to be reported, then I say it should be
reported later as well. Or seen the other way around, if it doesn't
need to be reported on later reads, then why bother reporting it on the
first read (rather than just returning 0)?

Roel's patch is correct and could be applied. That being said, I don't
like that kind of tricky code that needs to be explained to you. I'd
rather rewrite it in a simpler form which can be understood directly.
Especially given the fact that all the callers care about is boolean
success or failure. So I'd suggest the following code clean-up:

---
 drivers/hwmon/abituguru3.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- linux-2.6.29-rc5.orig/drivers/hwmon/abituguru3.c	2009-01-17 09:06:19.000000000 +0100
+++ linux-2.6.29-rc5/drivers/hwmon/abituguru3.c	2009-02-18 16:07:51.000000000 +0100
@@ -760,8 +760,11 @@ static int abituguru3_read_increment_off
 
 	for (i = 0; i < offset_count; i++)
 		if ((x = abituguru3_read(data, bank, offset + i, count,
-				buf + i * count)) != count)
-			return i * count + (i && (x < 0)) ? 0 : x;
+				buf + i * count)) != count) {
+			if (x < 0)
+				return x;
+			return i * count + x;
+		}
 
 	return i * count;
 }

Objections?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Wed, 18 Feb 2009 15:25:52 +0100, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> On Tue, 17 Feb 2009 21:03:23 +0000, Alistair John Strachan wrote:
>>>> This one's been there from the beginning.
>>>>
>>>> The change looks right to me. (i * count) account the previous successes, add 
>>>> either zero or x depending on the kind of read failure (timeout/short read vs 
>>>> other I/O error). I'm not entirely sure why i must be >0 for accounting the 
>>>> short read.
>>> I can't make any sense of that "i &&" either. Hans?
>> This is in an error handling path, if some bytes were read but not count bytes, 
>> then we return i * count + x, where i * count is the number of bytes read 
>> successful so far and x is the number of bytes read for the offset we were 
>> reading from when we got the short read.
>>
>> However if this is not just a short read, but a read which error-ed out then x 
>> < 0. If we then would return i * count + x, we would return less then what 
>> we've actually read. So in this case we want to return just i * count, which 
>> gets done in the form of i * count + 0.
> 
> So far so good...
> 
>> There is one special case however if we get an error code in the form of x < 0, 
>> and we've not done any successfull reads (so i = 0), then the right thing to 
>> do is to pass the error code along. So the condition in which we want to add 0 
>> to the result instead of x becomes (i && x < 0)
>>
>> Hopes this helps.
> 
> OK... I get the idea now. I'm glad you were still around to explain it
> because I would never have figured it out. And I don't really like the
> logic: behaving differently if the error occurs during the first read
> or the later ones doesn't make a lot of sense to me. If the error is so
> important that it needs to be reported, then I say it should be
> reported later as well. Or seen the other way around, if it doesn't
> need to be reported on later reads, then why bother reporting it on the
> first read (rather than just returning 0)?
> 
> Roel's patch is correct and could be applied. That being said, I don't
> like that kind of tricky code that needs to be explained to you. I'd
> rather rewrite it in a simpler form which can be understood directly.
> Especially given the fact that all the callers care about is boolean
> success or failure. So I'd suggest the following code clean-up:
> 
> ---
>  drivers/hwmon/abituguru3.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.29-rc5.orig/drivers/hwmon/abituguru3.c	2009-01-17 09:06:19.000000000 +0100
> +++ linux-2.6.29-rc5/drivers/hwmon/abituguru3.c	2009-02-18 16:07:51.000000000 +0100
> @@ -760,8 +760,11 @@ static int abituguru3_read_increment_off
>  
>  	for (i = 0; i < offset_count; i++)
>  		if ((x = abituguru3_read(data, bank, offset + i, count,
> -				buf + i * count)) != count)
> -			return i * count + (i && (x < 0)) ? 0 : x;
> +				buf + i * count)) != count) {
> +			if (x < 0)
> +				return x;
> +			return i * count + x;
> +		}
>  
>  	return i * count;
>  }
> 
> Objections?
> 

No, looks good to me!

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[alistair]

(Sorry for the delay and probable mail corruption, I'm travelling)

[snip]
>  	for (i = 0; i < offset_count; i++)
>  		if ((x = abituguru3_read(data, bank, offset + i, count,
> -				buf + i * count)) != count)
> -			return i * count + (i && (x < 0)) ? 0 : x;
> +				buf + i * count)) != count) {
> +			if (x < 0)
> +				return x;
> +			return i * count + x;
> +		}
>
>  	return i * count;
>  }
>
> Objections?

I have no objections to this patch.

However, if propagating errors up the stack is desirable, I think it would
also be prudent to fix abituguru3_update_device() to pass the error back
via ERR_PTR() or similar, instead of returning NULL. That way,
show_value(), show_alarm() and abituguru3_probe() could return f.e. -EIO,
generated by abituguru3_read().

This change is more involved; I can send a follow up for a later
time-frame if desired.

-- 
Cheers,
Alistair.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[Hans de Goede]

alistair@devzero.co.uk wrote:
> (Sorry for the delay and probable mail corruption, I'm travelling)
> 
> [snip]
>>  	for (i = 0; i < offset_count; i++)
>>  		if ((x = abituguru3_read(data, bank, offset + i, count,
>> -				buf + i * count)) != count)
>> -			return i * count + (i && (x < 0)) ? 0 : x;
>> +				buf + i * count)) != count) {
>> +			if (x < 0)
>> +				return x;
>> +			return i * count + x;
>> +		}
>>
>>  	return i * count;
>>  }
>>
>> Objections?
> 
> I have no objections to this patch.
> 
> However, if propagating errors up the stack is desirable, I think it would
> also be prudent to fix abituguru3_update_device() to pass the error back
> via ERR_PTR() or similar, instead of returning NULL. That way,
> show_value(), show_alarm() and abituguru3_probe() could return f.e. -EIO,
> generated by abituguru3_read().
> 
> This change is more involved; I can send a follow up for a later
> time-frame if desired.
> 

Note: I've never ever seen an actual IO-error with the uguru3 (unlike the 
revision 1), so i'm not sure this is worth your while.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: Add missing parentheses

[Jean Delvare]

On Thu, 19 Feb 2009 09:13:45 -0000 (GMT), alistair@devzero.co.uk wrote:
> (Sorry for the delay and probable mail corruption, I'm travelling)
> 
> [snip]
> >  	for (i = 0; i < offset_count; i++)
> >  		if ((x = abituguru3_read(data, bank, offset + i, count,
> > -				buf + i * count)) != count)
> > -			return i * count + (i && (x < 0)) ? 0 : x;
> > +				buf + i * count)) != count) {
> > +			if (x < 0)
> > +				return x;
> > +			return i * count + x;
> > +		}
> >
> >  	return i * count;
> >  }
> >
> > Objections?
> 
> I have no objections to this patch.

Thanks.

> However, if propagating errors up the stack is desirable, I think it would
> also be prudent to fix abituguru3_update_device() to pass the error back
> via ERR_PTR() or similar, instead of returning NULL. That way,
> show_value(), show_alarm() and abituguru3_probe() could return f.e. -EIO,
> generated by abituguru3_read().
> 
> This change is more involved; I can send a follow up for a later
> time-frame if desired.

I don't think it is really important. The read functions never return
any error other than -EIO anyway. And there is nothing user-space can do
about the error code. So I doubt it is worth spending time on this.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors