| Message ID | 20170712041647.GB29918@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jul 11, 2017 at 9:16 PM, Ben Pfaff <blp@ovn.org> wrote: > On Fri, Jun 23, 2017 at 09:49:19AM +0000, Kavanagh, Mark B wrote: >> >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of >> >Bhanuprakash Bodireddy >> >Sent: Monday, June 19, 2017 7:54 PM >> >To: dev@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@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@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@ovn.org> > > Thanks, > > Ben > > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <blp@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.bodireddy@intel.com> > CC: Andy Zhou <azhou@ovn.org> > Fixes: 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.") > Signed-off-by: Ben Pfaff <blp@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 >
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@ovn.org> wrote: > > On Fri, Jun 23, 2017 at 09:49:19AM +0000, Kavanagh, Mark B wrote: > >> >From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of > >> >Bhanuprakash Bodireddy > >> >Sent: Monday, June 19, 2017 7:54 PM > >> >To: dev@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@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@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@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 > >
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; + } + 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) {