Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1301031 > unrolled thread
| Started by | "Geyslan G. Bem" <geyslan@gmail.com> |
|---|---|
| First post | 2016-01-04 21:20 +0100 |
| Last post | 2016-01-04 23:00 +0100 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 07/17] usb: host: ehci-dbg: fix unsigned comparison "Geyslan G. Bem" <geyslan@gmail.com> - 2016-01-04 21:20 +0100
Re: [PATCH 07/17] usb: host: ehci-dbg: fix unsigned comparison Alan Stern <stern@rowland.harvard.edu> - 2016-01-04 22:00 +0100
Re: [PATCH 07/17] usb: host: ehci-dbg: fix unsigned comparison "Geyslan G. Bem" <geyslan@gmail.com> - 2016-01-04 22:40 +0100
Re: [PATCH 07/17] usb: host: ehci-dbg: fix unsigned comparison Alan Stern <stern@rowland.harvard.edu> - 2016-01-04 23:00 +0100
| From | "Geyslan G. Bem" <geyslan@gmail.com> |
|---|---|
| Date | 2016-01-04 21:20 +0100 |
| Subject | [PATCH 07/17] usb: host: ehci-dbg: fix unsigned comparison |
| Message-ID | <qNjLJ-8i4-27@gated-at.bofh.it> |
This patch fixes an unsigned comparison to less than 0.
Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
Notes:
I'm not sure about that comparison because in qh_lines() temp receives
the snprintf() return and thereafter occurs this comparison:
if (size < temp)
temp = size;
Is it a test of string truncation right? That possibility is being
treated. But if after some snprintf returns the temp is exactly size
minus 1 (trailing null)? Could this scenario happen? If yes, I think
size could be not counting correctly. Let me know more about it.
drivers/usb/host/ehci-dbg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
index 980ca55..1645120 100644
--- a/drivers/usb/host/ehci-dbg.c
+++ b/drivers/usb/host/ehci-dbg.c
@@ -542,7 +542,7 @@ static ssize_t fill_async_buffer(struct debug_buffer *buf)
next += temp;
list_for_each_entry(qh, &ehci->async_unlink, unlink_node) {
- if (size <= 0)
+ if (size == 0)
break;
qh_lines(ehci, qh, &next, &size);
}
--
2.6.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-01-04 22:00 +0100 |
| Message-ID | <qNkoq-6b-1@gated-at.bofh.it> |
| In reply to | #1301031 |
On Mon, 4 Jan 2016, Geyslan G. Bem wrote:
> This patch fixes an unsigned comparison to less than 0.
No, it doesn't. It changes an unsigned comparison for less than or
equal to 0, which is very different from less than 0.
> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> ---
>
> Notes:
> I'm not sure about that comparison because in qh_lines() temp receives
> the snprintf() return and thereafter occurs this comparison:
>
> if (size < temp)
> temp = size;
>
> Is it a test of string truncation right? That possibility is being
> treated. But if after some snprintf returns the temp is exactly size
> minus 1 (trailing null)? Could this scenario happen? If yes, I think
> size could be not counting correctly. Let me know more about it.
I think the two weird code sequences in qh_lines() were written before
scnprintf existed. They should be changed to use scnprintf instead of
snprintf; then the "if (size < temp) temp = size;" things can be
removed.
> drivers/usb/host/ehci-dbg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> index 980ca55..1645120 100644
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -542,7 +542,7 @@ static ssize_t fill_async_buffer(struct debug_buffer *buf)
> next += temp;
>
> list_for_each_entry(qh, &ehci->async_unlink, unlink_node) {
> - if (size <= 0)
> + if (size == 0)
> break;
> qh_lines(ehci, qh, &next, &size);
> }
The new line does exactly the same thing as the old line. There's no
reason to make this change.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | "Geyslan G. Bem" <geyslan@gmail.com> |
|---|---|
| Date | 2016-01-04 22:40 +0100 |
| Message-ID | <qNl19-An-23@gated-at.bofh.it> |
| In reply to | #1301085 |
2016-01-04 17:50 GMT-03:00 Alan Stern <stern@rowland.harvard.edu>:
> On Mon, 4 Jan 2016, Geyslan G. Bem wrote:
>
>> This patch fixes an unsigned comparison to less than 0.
>
> No, it doesn't. It changes an unsigned comparison for less than or
> equal to 0, which is very different from less than 0.
You're right. The statemant is incomplete.
>
>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> ---
>>
>> Notes:
>> I'm not sure about that comparison because in qh_lines() temp receives
>> the snprintf() return and thereafter occurs this comparison:
>>
>> if (size < temp)
>> temp = size;
>>
>> Is it a test of string truncation right? That possibility is being
>> treated. But if after some snprintf returns the temp is exactly size
>> minus 1 (trailing null)? Could this scenario happen? If yes, I think
>> size could be not counting correctly. Let me know more about it.
>
> I think the two weird code sequences in qh_lines() were written before
> scnprintf existed. They should be changed to use scnprintf instead of
> snprintf; then the "if (size < temp) temp = size;" things can be
> removed.
I see. I can do another patch for that if you allow me.
>
>> drivers/usb/host/ehci-dbg.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
>> index 980ca55..1645120 100644
>> --- a/drivers/usb/host/ehci-dbg.c
>> +++ b/drivers/usb/host/ehci-dbg.c
>> @@ -542,7 +542,7 @@ static ssize_t fill_async_buffer(struct debug_buffer *buf)
>> next += temp;
>>
>> list_for_each_entry(qh, &ehci->async_unlink, unlink_node) {
>> - if (size <= 0)
>> + if (size == 0)
>> break;
>> qh_lines(ehci, qh, &next, &size);
>> }
>
> The new line does exactly the same thing as the old line. There's no
> reason to make this change.
I think that the original and new logic will be the same because the
size variable has no sign. If in some previous subtraction the
subtracted value is greater than size value, this will spin (rotate),
probably, to a great positive value.
The compiled code will not change indeed. That change was only focused
on the improvement of the code reading. So if you allow me I could
change the commit message. If not let's forget it. :-)
>
> Alan Stern
>
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2016-01-04 23:00 +0100 |
| Message-ID | <qNlku-HP-7@gated-at.bofh.it> |
| In reply to | #1301115 |
On Mon, 4 Jan 2016, Geyslan G. Bem wrote:
> 2016-01-04 17:50 GMT-03:00 Alan Stern <stern@rowland.harvard.edu>:
> > On Mon, 4 Jan 2016, Geyslan G. Bem wrote:
> >
> >> This patch fixes an unsigned comparison to less than 0.
> >
> > No, it doesn't. It changes an unsigned comparison for less than or
> > equal to 0, which is very different from less than 0.
> You're right. The statemant is incomplete.
>
> >
> >> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> >> ---
> >>
> >> Notes:
> >> I'm not sure about that comparison because in qh_lines() temp receives
> >> the snprintf() return and thereafter occurs this comparison:
> >>
> >> if (size < temp)
> >> temp = size;
> >>
> >> Is it a test of string truncation right? That possibility is being
> >> treated. But if after some snprintf returns the temp is exactly size
> >> minus 1 (trailing null)? Could this scenario happen? If yes, I think
> >> size could be not counting correctly. Let me know more about it.
> >
> > I think the two weird code sequences in qh_lines() were written before
> > scnprintf existed. They should be changed to use scnprintf instead of
> > snprintf; then the "if (size < temp) temp = size;" things can be
> > removed.
> I see. I can do another patch for that if you allow me.
Sure, go ahead.
> >> drivers/usb/host/ehci-dbg.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> >> index 980ca55..1645120 100644
> >> --- a/drivers/usb/host/ehci-dbg.c
> >> +++ b/drivers/usb/host/ehci-dbg.c
> >> @@ -542,7 +542,7 @@ static ssize_t fill_async_buffer(struct debug_buffer *buf)
> >> next += temp;
> >>
> >> list_for_each_entry(qh, &ehci->async_unlink, unlink_node) {
> >> - if (size <= 0)
> >> + if (size == 0)
> >> break;
> >> qh_lines(ehci, qh, &next, &size);
> >> }
> >
> > The new line does exactly the same thing as the old line. There's no
> > reason to make this change.
> I think that the original and new logic will be the same because the
> size variable has no sign. If in some previous subtraction the
> subtracted value is greater than size value, this will spin (rotate),
> probably, to a great positive value.
>
> The compiled code will not change indeed. That change was only focused
> on the improvement of the code reading. So if you allow me I could
> change the commit message. If not let's forget it. :-)
IMO there's no improvement in reading the code. So let's forget about
this.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web