Re: [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 [off-list ref] wrote:
      From: Arushi Singhal [off-list ref]
      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
      [off-list ref]
      > ---
      > 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.