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, > > Ben > > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <b...@ovn.org> > Date: Tue, 11 Jul 2017 21:15:21 -0700 > Subject: [PATCH] test-conntrack: Restore packet batching to pcap test. > > The test accepted but then ignored the batch count argument. > > Reported-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com> > CC: Andy Zhou <az...@ovn.org> > Fixes: 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.") > Signed-off-by: Ben Pfaff <b...@ovn.org> > --- > tests/test-conntrack.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c > index f79a9fcc61e3..6a77db8df5ae 100644 > --- a/tests/test-conntrack.c > +++ b/tests/test-conntrack.c > @@ -211,17 +211,22 @@ test_pcap(struct ovs_cmdl_context *ctx) > > conntrack_init(&ct); > total_count = 0; > - while (!err) { > + for (;;) { > struct dp_packet *packet; > struct dp_packet_batch pkt_batch_; > struct dp_packet_batch *batch = &pkt_batch_; > > dp_packet_batch_init(batch); > - err = ovs_pcap_read(pcap, &packet, NULL); > - if (err) { > + 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'? > + } > + 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