On Wed, 7 Mar 2018, Arushi Singhal wrote:
> > > On Wed, Mar 7, 2018 at 9:09 PM, David Miller <da...@davemloft.net> wrote: > From: Arushi Singhal <arushisinghal19971...@gmail.com> > Date: Sat, 3 Mar 2018 21:44:56 +0530 > > > Continue at the bottom of a loop are removed. > > Issue found using drop_continue.cocci Coccinelle script. > > > > Signed-off-by: Arushi Singhal > <arushisinghal19971...@gmail.com> > > --- > > Changes in v2: > > - Braces is dropped from if with single statement. > > Sorry there is no way I am applying this. > > Just blindly removing continue statements that some static > checker > or compiler warning mentions is completely unacceptable. > > Actually _LOOK_ at this code: > > > if(cards[i].vendor_id) { > > for(j=0;j<3;j++) > > - > if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j]) > { > > + > if(inb(ioaddr+cards[i].addr_offset+j) != cards[i].vendor_id[j]) > > release_region(ioaddr, > cards[i].total_size); > > - continue; > > - } > > } > > The extraneous continue statement is the _least_ of the problems > here. > > > Hello David > > Yes you are correct. > > > This code, if the if() statement triggers, will release the > ioaddr > region and then _CONTINUE_ the for(j) loop, and try to access > the > registers using the same 'ioaddr' again. > > That's actually a _REAL_ bug, and you're making it seem like > the behavior is intentional by editing it like this. > > And the bug probably exists because this entire sequence has > misaligned closing curly braces. It makes it look like > the continue is for the outer loop, which would be fine. > > But it's not. It's for the inner loop, and this causes the "use > ioaddr after releasing it" bug. > > > Yes I know it's for the inner loop. > But I am not able to find, where I am wrong here Arushi, You are preserving the current behavior and David is telling you that the current behavior is incorrect. He doesn't want a patch that changes the code but preesrves the current (wrong) behavior, because that somehow contributes to the illusion that the incorrect code is correct. julia > > For example > BEFORE:- > for(i=1;i<10;i++) > if (expr1) > { > statement; > continue; > } > > After My Patch > for(i=1;i<10;i++) > if (expr1) > statement; > > I am not understanding where I am getting wrong in understanding this. > For me both are equivalent. > > I Agree with Jakub reply:- > "This is an error handling path so the continue makes sense here to > indicate the processing can't ever fall through if more statements are > ever added to the loop. But OK." > > Thanks > Arushi > > > Please do not submit patches like this one, it makes for a lot > of > wasted auditing and review for people. The onus is on you to > read > and understand the code you are editing before submitting your > changes. > > Thank you. > > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web > visithttps://groups.google.com/d/msgid/outreachy-kernel/CA%2BXqjF8_axeTnahPmokxm > Dnm4NmWo8QWSEmLejN4fO1nAiDaBA%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout. > >