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

Andy Zhou azhou at ovn.org
Wed Jul 12 20:52:50 UTC 2017


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,
>
> Ben
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Ben Pfaff <blp at 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 at intel.com>
> CC: Andy Zhou <azhou at ovn.org>
> Fixes: 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.")
> Signed-off-by: Ben Pfaff <blp at 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
>


More information about the dev mailing list