* fs/jffs2/readinode.c:189: faulty logic ?
@ 2017-01-24 8:15 David Binderman
2017-01-24 14:52 ` Marek Vasut
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: David Binderman @ 2017-01-24 8:15 UTC (permalink / raw)
To: dwmw2, linux-mtd, linux-kernel
Hello there,
fs/jffs2/readinode.c:189]: (style) Condition 'tn.fn.ofs>=offset' is always true
Source code is
if (tn->fn->ofs < offset)
next = tn->rb.rb_right;
else if (tn->fn->ofs >= offset)
next = tn->rb.rb_left;
else
break;
Maybe better code
if (tn->fn->ofs < offset)
next = tn->rb.rb_right;
else if (tn->fn->ofs > offset)
next = tn->rb.rb_left;
else
break;
Regards
David Binderman
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: fs/jffs2/readinode.c:189: faulty logic ?
2017-01-24 8:15 fs/jffs2/readinode.c:189: faulty logic ? David Binderman
@ 2017-01-24 14:52 ` Marek Vasut
2017-01-24 15:11 ` Joakim Tjernlund
[not found] ` <VI1PR08MB1022B30CF5D9834EC9304F0F9C750@VI1PR08MB1022.eurprd08.prod.outlook.com>
2017-01-24 22:20 ` Richard Weinberger
2017-01-24 23:30 ` David Woodhouse
2 siblings, 2 replies; 8+ messages in thread
From: Marek Vasut @ 2017-01-24 14:52 UTC (permalink / raw)
To: David Binderman, dwmw2, linux-mtd, linux-kernel
On 01/24/2017 09:15 AM, David Binderman wrote:
> Hello there,
>
> fs/jffs2/readinode.c:189]: (style) Condition 'tn.fn.ofs>=offset' is always true
>
> Source code is
>
> if (tn->fn->ofs < offset)
> next = tn->rb.rb_right;
> else if (tn->fn->ofs >= offset)
> next = tn->rb.rb_left;
> else
> break;
>
> Maybe better code
>
> if (tn->fn->ofs < offset)
> next = tn->rb.rb_right;
> else if (tn->fn->ofs > offset)
> next = tn->rb.rb_left;
> else
> break;
This changes the logic of the code for equality case, please elaborate
why this is OK.
> Regards
>
> David Binderman
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: fs/jffs2/readinode.c:189: faulty logic ?
2017-01-24 14:52 ` Marek Vasut
@ 2017-01-24 15:11 ` Joakim Tjernlund
2017-01-24 15:14 ` Marek Vasut
[not found] ` <VI1PR08MB1022B30CF5D9834EC9304F0F9C750@VI1PR08MB1022.eurprd08.prod.outlook.com>
1 sibling, 1 reply; 8+ messages in thread
From: Joakim Tjernlund @ 2017-01-24 15:11 UTC (permalink / raw)
To: linux-mtd, linux-kernel, marek.vasut, dwmw2, dcb314
On Tue, 2017-01-24 at 15:52 +0100, Marek Vasut wrote:
> On 01/24/2017 09:15 AM, David Binderman wrote:
> > Hello there,
> >
> > fs/jffs2/readinode.c:189]: (style) Condition 'tn.fn.ofs>=offset' is always true
> >
> > Source code is
> >
> > if (tn->fn->ofs < offset)
> > next = tn->rb.rb_right;
> > else if (tn->fn->ofs >= offset)
> > next = tn->rb.rb_left;
> > else
> > break;
> >
> > Maybe better code
> >
> > if (tn->fn->ofs < offset)
> > next = tn->rb.rb_right;
> > else if (tn->fn->ofs > offset)
> > next = tn->rb.rb_left;
> > else
> > break;
>
> This changes the logic of the code for equality case, please elaborate
> why this is OK.
There is something odd with current code:
next = tn_root->rb_node;
while (next) {
tn = rb_entry(next, struct jffs2_tmp_dnode_info, rb);
if (tn->fn->ofs < offset)
next = tn->rb.rb_right;
else if (tn->fn->ofs >= offset)
next = tn->rb.rb_left;
else
break;
}
The else break; is never reached so the above change makes the break work for ==
Weather this is correct or not I cannot say.
Jocke
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: fs/jffs2/readinode.c:189: faulty logic ?
2017-01-24 15:11 ` Joakim Tjernlund
@ 2017-01-24 15:14 ` Marek Vasut
0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2017-01-24 15:14 UTC (permalink / raw)
To: Joakim Tjernlund, linux-mtd, linux-kernel, dwmw2, dcb314
On 01/24/2017 04:11 PM, Joakim Tjernlund wrote:
> On Tue, 2017-01-24 at 15:52 +0100, Marek Vasut wrote:
>> On 01/24/2017 09:15 AM, David Binderman wrote:
>>> Hello there,
>>>
>>> fs/jffs2/readinode.c:189]: (style) Condition 'tn.fn.ofs>=offset' is always true
>>>
>>> Source code is
>>>
>>> if (tn->fn->ofs < offset)
>>> next = tn->rb.rb_right;
>>> else if (tn->fn->ofs >= offset)
>>> next = tn->rb.rb_left;
>>> else
>>> break;
>>>
>>> Maybe better code
>>>
>>> if (tn->fn->ofs < offset)
>>> next = tn->rb.rb_right;
>>> else if (tn->fn->ofs > offset)
>>> next = tn->rb.rb_left;
>>> else
>>> break;
>>
>> This changes the logic of the code for equality case, please elaborate
>> why this is OK.
>
> There is something odd with current code:
> next = tn_root->rb_node;
>
> while (next) {
> tn = rb_entry(next, struct jffs2_tmp_dnode_info, rb);
>
> if (tn->fn->ofs < offset)
> next = tn->rb.rb_right;
> else if (tn->fn->ofs >= offset)
> next = tn->rb.rb_left;
> else
> break;
> }
>
> The else break; is never reached so the above change makes the break work for ==
> Weather this is correct or not I cannot say.
Indeed, I agree with this one. This is why I asked for more detailed
explanation too :)
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: fs/jffs2/readinode.c:189: faulty logic ?
[not found] ` <VI1PR08MB1022B30CF5D9834EC9304F0F9C750@VI1PR08MB1022.eurprd08.prod.outlook.com>
@ 2017-01-24 19:59 ` Marek Vasut
0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2017-01-24 19:59 UTC (permalink / raw)
To: David Binderman, dwmw2, linux-mtd, linux-kernel
On 01/24/2017 08:17 PM, David Binderman wrote:
>
> Hello there,
>
>
> ________________________________
>
>> This changes the logic of the code for equality case, please elaborate
>> why this is OK.
>
> RTFM.
>
> Linux kernel file Documentation/rbtree.txt, around line 95, has the standard logic
> for traversing a red black tree.
In that case, please RTFM Documentation/process/submitting-patches.rst
and we continue the discussion there :)
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: fs/jffs2/readinode.c:189: faulty logic ?
2017-01-24 8:15 fs/jffs2/readinode.c:189: faulty logic ? David Binderman
2017-01-24 14:52 ` Marek Vasut
@ 2017-01-24 22:20 ` Richard Weinberger
[not found] ` <VI1PR08MB1022FF07BD9A4B08984561FE9C750@VI1PR08MB1022.eurprd08.prod.outlook.com>
2017-01-24 23:30 ` David Woodhouse
2 siblings, 1 reply; 8+ messages in thread
From: Richard Weinberger @ 2017-01-24 22:20 UTC (permalink / raw)
To: David Binderman; +Cc: dwmw2, linux-mtd, linux-kernel
David,
On Tue, Jan 24, 2017 at 9:15 AM, David Binderman <dcb314@hotmail.com> wrote:
> Hello there,
>
> fs/jffs2/readinode.c:189]: (style) Condition 'tn.fn.ofs>=offset' is always true
What tool produces this info?
> Source code is
>
> if (tn->fn->ofs < offset)
> next = tn->rb.rb_right;
> else if (tn->fn->ofs >= offset)
> next = tn->rb.rb_left;
> else
> break;
>
> Maybe better code
>
> if (tn->fn->ofs < offset)
> next = tn->rb.rb_right;
> else if (tn->fn->ofs > offset)
> next = tn->rb.rb_left;
> else
> break;
That code is odd, yes. But we need more info.
Why is fn.fn.ofs>=offset always true?
AFAICT tn->fn->ofs > offset can be also true but
else break is never reached.
--
Thanks,
//richard
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: fs/jffs2/readinode.c:189: faulty logic ?
2017-01-24 8:15 fs/jffs2/readinode.c:189: faulty logic ? David Binderman
2017-01-24 14:52 ` Marek Vasut
2017-01-24 22:20 ` Richard Weinberger
@ 2017-01-24 23:30 ` David Woodhouse
2 siblings, 0 replies; 8+ messages in thread
From: David Woodhouse @ 2017-01-24 23:30 UTC (permalink / raw)
To: David Binderman, linux-mtd, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1035 bytes --]
On Tue, 2017-01-24 at 08:15 +0000, David Binderman wrote:
> Hello there,
>
> fs/jffs2/readinode.c:189]: (style) Condition 'tn.fn.ofs>=offset' is
> always true
>
> Source code is
>
> if (tn->fn->ofs < offset)
> next = tn->rb.rb_right;
> else if (tn->fn->ofs >= offset)
> next = tn->rb.rb_left;
> else
> break;
>
> Maybe better code
>
> if (tn->fn->ofs < offset)
> next = tn->rb.rb_right;
> else if (tn->fn->ofs > offset)
> next = tn->rb.rb_left;
> else
> break;
Thanks for pointing this out; it looks like a correct fix at first.
However, it might expose a bug, if the 'break' case isn't particularly
well-tested. The existing bug is probably fairly harmless, while the
hypothetical new one isn't. So we need to take a closer look at the
surrounding code and ideally test it...
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4938 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: fs/jffs2/readinode.c:189: faulty logic ?
[not found] ` <VI1PR08MB1022FF07BD9A4B08984561FE9C750@VI1PR08MB1022.eurprd08.prod.outlook.com>
@ 2017-01-25 8:31 ` Richard Weinberger
0 siblings, 0 replies; 8+ messages in thread
From: Richard Weinberger @ 2017-01-25 8:31 UTC (permalink / raw)
To: David Binderman, Richard Weinberger; +Cc: dwmw2, linux-mtd, linux-kernel
David,
Am 24.01.2017 um 23:57 schrieb David Binderman:
>>> fs/jffs2/readinode.c:189]: (style) Condition 'tn.fn.ofs>=offset' is always true
>
>>What tool produces this info?
>
> A static analyser for C and C++ called cppcheck, available from sourceforge.
That needs to go into the changelog.
>>That code is odd, yes. But we need more info.
>>Why is fn.fn.ofs>=offset always true?
>
> Doh. By obvious inspection of the code.
> If test x < y fails, then x must be >= y.
So tn.fn.ofs>=offset is _not_ always true.
The else branch is always false.
Thanks,
//richard
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-25 8:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-24 8:15 fs/jffs2/readinode.c:189: faulty logic ? David Binderman
2017-01-24 14:52 ` Marek Vasut
2017-01-24 15:11 ` Joakim Tjernlund
2017-01-24 15:14 ` Marek Vasut
[not found] ` <VI1PR08MB1022B30CF5D9834EC9304F0F9C750@VI1PR08MB1022.eurprd08.prod.outlook.com>
2017-01-24 19:59 ` Marek Vasut
2017-01-24 22:20 ` Richard Weinberger
[not found] ` <VI1PR08MB1022FF07BD9A4B08984561FE9C750@VI1PR08MB1022.eurprd08.prod.outlook.com>
2017-01-25 8:31 ` Richard Weinberger
2017-01-24 23:30 ` David Woodhouse
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.