[Outreachy kernel] Re: [PATCH v2] net: ethernet: Drop unnecessary continue

[Arushi Singhal <arushisinghal19971997@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3199 bytes --]

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

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 visit https://groups.google.com/d/msgid/outreachy-kernel/CA%2BXqjF8_axeTnahPmokxmDnm4NmWo8QWSEmLejN4fO1nAiDaBA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 5123 bytes --]