On Wed, Jul 12, 2017 at 01:52:50PM -0700, Andy Zhou wrote: > On Tue, Jul 11, 2017 at 9:16 PM, Ben Pfaff <b...@ovn.org> wrote: > > On Fri, Jun 23, 2017 at 09:49:19AM +0000, Kavanagh, Mark B wrote: > >> >From: ovs-dev-boun...@openvswitch.org > >> >[mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of > >> >Bhanuprakash Bodireddy > >> >Sent: Monday, June 19, 2017 7:54 PM > >> >To: d...@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.bodire...@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.kavan...@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 <az...@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 > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev