From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julia Lawall Subject: Re: [Outreachy kernel] Re: [PATCH v2] net: ethernet: Drop unnecessary continue Date: Wed, 7 Mar 2018 20:41:29 +0100 (CET) Message-ID: References: <20180303161456.GA20194@seema-Inspiron-15-3567> <20180307.103901.1771176275743006915.davem@davemloft.net> Mime-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="8323329-1036440338-1520451689=:2814" Cc: David Miller , jdmason@kudzu.us, Jakub Kicinski , Linux Kernel Network Developers , LKML , oss-drivers@netronome.com, outreachy-kernel@googlegroups.com To: Arushi Singhal Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-1036440338-1520451689=:2814 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Wed, 7 Mar 2018, Arushi Singhal wrote: > > > On Wed, Mar 7, 2018 at 9:09 PM, David Miller wrote: > From: Arushi Singhal > 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 > > > --- > > 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+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@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. > > --8323329-1036440338-1520451689=:2814--