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

[Julia Lawall]

On Wed, 7 Mar 2018, Arushi Singhal wrote:

>
>
> On Wed, Mar 7, 2018 at 9:09 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:
>       From: Arushi Singhal <arushisinghal19971997@xxxxxxxxx>
>       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@xxxxxxxxx>
>       > ---
>       > 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@xxxxxxxxxxxxxxxxx
> To post to this group, send email to outreachy-kernel@xxxxxxxxxxxxxxxxx
> 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.
>
>