[ovs-dev] [PATCH 5/6] test-conntrack: Fix dead store reported by clang.

[Ben Pfaff]

On Wed, Jul 12, 2017 at 01:52:50PM -0700, Andy Zhou wrote:
> On Tue, Jul 11, 2017 at 9:16 PM, Ben Pfaff <blp at ovn.org> wrote:
> > On Fri, Jun 23, 2017 at 09:49:19AM +0000, Kavanagh, Mark B wrote:
> >> >From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of
> >> >Bhanuprakash Bodireddy
> >> >Sent: Monday, June 19, 2017 7:54 PM
> >> >To: dev at openvswitch.org
> >> >Subject: [ovs-dev] [PATCH 5/6] test-conntrack: Fix dead store reported by clang.
> >> >
> >> >Clang reports that value store to 'batch_size' is never read.
> >> >
> >> >Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
> >>
> >> Hi Bhanu,
> >>
> >> LGTM - I also compiled this with gcc, clang, and sparse without issue. Checkpatch reports no obvious problems either.
> >>
> >> Acked-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
> >
> > Thanks for noticing the problem.
> >
> > Even with this change, batch_size is a write-only variable.  Nothing
> > ever uses it.
> >
> > I think that the right fix is something more like this.  I have not
> > tested it.
> >
> > Andy, it looks like you made the previous change here, does this make
> > sense?
> 
> I think this version is closer to the original intent of the test
> interface than what I have changed.
> Here is my Ack for this change.  I have a minor comment below.
> 
> Acked-by: Andy Zhou <azhou at ovn.org>

Thanks a lot for the review.

> > +        for (int i = 0; i < batch_size; i++) {
> > +            err = ovs_pcap_read(pcap, &packet, NULL);
> > +            if (err) {
> > +                break;
> Would it make sense to use 'continue' here instead of 'break'?

I guess that ordinarily if there's a read error (or, more commonly, "end
of file"), a later read won't succeed, so I don't see much value in
that.  I think it's also not worth over-thinking this for a test
program.

I applied this to master.

> > +            }
> > +            dp_packet_batch_add(batch, packet);
> > +        }
> > +        if (!batch->count) {
> >              break;
> >          }
> > -        dp_packet_batch_add(batch, packet);
> >          pcap_batch_execute_conntrack(&ct, batch);
> >
> >          DP_PACKET_BATCH_FOR_EACH (packet, batch) {
> > --
> > 2.10.2
> >