From: Julia Lawall <julia.lawall@lip6.fr>
To: Arushi Singhal <arushisinghal19971997@gmail.com>
Cc: David Miller <davem@davemloft.net>,
jdmason@kudzu.us, Jakub Kicinski <jakub.kicinski@netronome.com>,
Linux Kernel Network Developers <netdev@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
oss-drivers@netronome.com, outreachy-kernel@googlegroups.com
Subject: Re: [Outreachy kernel] Re: [PATCH v2] net: ethernet: Drop unnecessary continue
Date: Wed, 7 Mar 2018 20:41:29 +0100 (CET) [thread overview]
Message-ID: <alpine.DEB.2.20.1803072039260.2814@hadrien> (raw)
In-Reply-To: <CA+XqjF8_axeTnahPmokxmDnm4NmWo8QWSEmLejN4fO1nAiDaBA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4756 bytes --]
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.
>
>
--
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/alpine.DEB.2.20.1803072039260.2814%40hadrien.
For more options, visit https://groups.google.com/d/optout.
next prev parent reply other threads:[~2018-03-07 19:41 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-03 16:14 [Outreachy kernel] [PATCH v2] net: ethernet: Drop unnecessary continue Arushi Singhal
2018-03-05 1:57 ` [Outreachy kernel] " Jakub Kicinski
2018-03-07 15:39 ` David Miller
2018-03-07 16:52 ` Arushi Singhal
2018-03-07 17:01 ` David Miller
2018-03-07 19:41 ` Julia Lawall [this message]
2018-03-08 2:57 ` Arushi Singhal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.20.1803072039260.2814@hadrien \
--to=julia.lawall@lip6.fr \
--cc=arushisinghal19971997@gmail.com \
--cc=davem@davemloft.net \
--cc=jakub.kicinski@netronome.com \
--cc=jdmason@kudzu.us \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=oss-drivers@netronome.com \
--cc=outreachy-kernel@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).