lkml.org 
[lkml]   [2018]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Outreachy kernel] Re: [PATCH v2] net: ethernet: Drop unnecessary continue


On Wed, 7 Mar 2018, Arushi Singhal wrote:

>
>
> On Wed, Mar 7, 2018 at 9:09 PM, David Miller <davem@davemloft.net> wrote:
> From: Arushi Singhal <arushisinghal19971997@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
> <arushisinghal19971997@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+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.
>
>
\
 
 \ /
  Last update: 2018-03-07 20:42    [W:0.090 / U:10.260 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site