[v2,2/4] test/ring: fix wrong size used in memcmp
Checks
Commit Message
When using memcmp function to check data, the third param should be the
size of all elements, rather than the number of the elements.
Fixes: a9fe152363e2 ("test/ring: add custom element size functional tests")
Cc: honnappa.nagarahalli@arm.com
Cc: stable@dpdk.org
Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
app/test/test_ring.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
Comments
<snip>
>
> When using memcmp function to check data, the third param should be the
> size of all elements, rather than the number of the elements.
>
> Fixes: a9fe152363e2 ("test/ring: add custom element size functional tests")
> Cc: honnappa.nagarahalli@arm.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> app/test/test_ring.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/app/test/test_ring.c b/app/test/test_ring.c index
> 0ae97d341..c508a13a9 100644
> --- a/app/test/test_ring.c
> +++ b/app/test/test_ring.c
> @@ -444,7 +444,12 @@ test_ring_burst_bulk_tests1(unsigned int test_idx)
> TEST_RING_VERIFY(rte_ring_empty(r));
>
> /* check data */
> - TEST_RING_VERIFY(memcmp(src, dst, rsz) == 0);
> + if (esize[i] == -1) {
> + TEST_RING_VERIFY(memcmp(src, dst,
> + rsz * sizeof(void *)) == 0);
> + } else
> + TEST_RING_VERIFY(memcmp(src, dst,
> + rsz * esize[i]) == 0);
Can you implement a function similar to 'test_ring_mem_init' to do this comparison?
> }
>
> /* Free memory before test completed */ @@ -538,9 +543,11
> @@ test_ring_burst_bulk_tests2(unsigned int test_idx)
> cur_dst = test_ring_inc_ptr(cur_dst, esize[i], MAX_BULK);
>
> /* check data */
> - if (memcmp(src, dst, cur_dst - dst)) {
> - rte_hexdump(stdout, "src", src, cur_src - src);
> - rte_hexdump(stdout, "dst", dst, cur_dst - dst);
> + if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
> + rte_hexdump(stdout, "src", src,
> + RTE_PTR_DIFF(cur_src, src));
> + rte_hexdump(stdout, "dst", dst,
> + RTE_PTR_DIFF(cur_dst, dst));
I do not think, this change and the rest below are bug fixes. Can you please separate them into another commit?
> printf("data after dequeue is not the same\n");
> goto fail;
> }
> @@ -614,9 +621,11 @@ test_ring_burst_bulk_tests3(unsigned int test_idx)
> }
>
> /* check data */
> - if (memcmp(src, dst, cur_dst - dst)) {
> - rte_hexdump(stdout, "src", src, cur_src - src);
> - rte_hexdump(stdout, "dst", dst, cur_dst - dst);
> + if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
> + rte_hexdump(stdout, "src", src,
> + RTE_PTR_DIFF(cur_src, src));
> + rte_hexdump(stdout, "dst", dst,
> + RTE_PTR_DIFF(cur_dst, dst));
> printf("data after dequeue is not the same\n");
> goto fail;
> }
> @@ -747,9 +756,11 @@ test_ring_burst_bulk_tests4(unsigned int test_idx)
> goto fail;
>
> /* check data */
> - if (memcmp(src, dst, cur_dst - dst)) {
> - rte_hexdump(stdout, "src", src, cur_src - src);
> - rte_hexdump(stdout, "dst", dst, cur_dst - dst);
> + if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
> + rte_hexdump(stdout, "src", src,
> + RTE_PTR_DIFF(cur_src, src));
> + rte_hexdump(stdout, "dst", dst,
> + RTE_PTR_DIFF(cur_dst, dst));
> printf("data after dequeue is not the same\n");
> goto fail;
> }
> --
> 2.17.1
> -----邮件原件-----
> 发件人: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> 发送时间: 2020年8月27日 4:52
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; Konstantin Ananyev
> <konstantin.ananyev@intel.com>; Gavin Hu <Gavin.Hu@arm.com>; Olivier
> Matz <olivier.matz@6wind.com>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; Feifei Wang
> <Feifei.Wang2@arm.com>; stable@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> 主题: RE: [PATCH v2 2/4] test/ring: fix wrong size used in memcmp
>
> <snip>
>
> >
> > When using memcmp function to check data, the third param should be
> > the size of all elements, rather than the number of the elements.
> >
> > Fixes: a9fe152363e2 ("test/ring: add custom element size functional
> > tests")
> > Cc: honnappa.nagarahalli@arm.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > app/test/test_ring.c | 31 +++++++++++++++++++++----------
> > 1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > diff --git a/app/test/test_ring.c b/app/test/test_ring.c index
> > 0ae97d341..c508a13a9 100644
> > --- a/app/test/test_ring.c
> > +++ b/app/test/test_ring.c
> > @@ -444,7 +444,12 @@ test_ring_burst_bulk_tests1(unsigned int test_idx)
> > TEST_RING_VERIFY(rte_ring_empty(r));
> >
> > /* check data */
> > - TEST_RING_VERIFY(memcmp(src, dst, rsz) == 0);
> > + if (esize[i] == -1) {
> > + TEST_RING_VERIFY(memcmp(src, dst,
> > + rsz * sizeof(void *)) == 0);
> > + } else
> > + TEST_RING_VERIFY(memcmp(src, dst,
> > + rsz * esize[i]) == 0);
> Can you implement a function similar to 'test_ring_mem_init' to do this
> comparison?
Ok, the new function named 'test_ring_mem_cmp' will be added.
>
> > }
> >
> > /* Free memory before test completed */ @@ -538,9 +543,11
> @@
> > test_ring_burst_bulk_tests2(unsigned int test_idx)
> > cur_dst = test_ring_inc_ptr(cur_dst, esize[i], MAX_BULK);
> >
> > /* check data */
> > - if (memcmp(src, dst, cur_dst - dst)) {
> > - rte_hexdump(stdout, "src", src, cur_src - src);
> > - rte_hexdump(stdout, "dst", dst, cur_dst - dst);
> > + if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
> > + rte_hexdump(stdout, "src", src,
> > + RTE_PTR_DIFF(cur_src, src));
> > + rte_hexdump(stdout, "dst", dst,
> > + RTE_PTR_DIFF(cur_dst, dst));
> I do not think, this change and the rest below are bug fixes. Can you please
> separate them into another commit?
Actually, in the original code, 'memcmp' wants to check all bytes and ' rte_hexdump ' wants to print all bytes.
However, 'cur_dst - dst' and 'cur_src - src' is the number of all elements rather the bytes of all elements.
As a result, we need to correct it and make them check all bytes of the ring.
>
> > printf("data after dequeue is not the same\n");
> > goto fail;
> > }
> > @@ -614,9 +621,11 @@ test_ring_burst_bulk_tests3(unsigned int test_idx)
> > }
> >
> > /* check data */
> > - if (memcmp(src, dst, cur_dst - dst)) {
> > - rte_hexdump(stdout, "src", src, cur_src - src);
> > - rte_hexdump(stdout, "dst", dst, cur_dst - dst);
> > + if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
> > + rte_hexdump(stdout, "src", src,
> > + RTE_PTR_DIFF(cur_src, src));
> > + rte_hexdump(stdout, "dst", dst,
> > + RTE_PTR_DIFF(cur_dst, dst));
> > printf("data after dequeue is not the same\n");
> > goto fail;
> > }
> > @@ -747,9 +756,11 @@ test_ring_burst_bulk_tests4(unsigned int test_idx)
> > goto fail;
> >
> > /* check data */
> > - if (memcmp(src, dst, cur_dst - dst)) {
> > - rte_hexdump(stdout, "src", src, cur_src - src);
> > - rte_hexdump(stdout, "dst", dst, cur_dst - dst);
> > + if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
> > + rte_hexdump(stdout, "src", src,
> > + RTE_PTR_DIFF(cur_src, src));
> > + rte_hexdump(stdout, "dst", dst,
> > + RTE_PTR_DIFF(cur_dst, dst));
> > printf("data after dequeue is not the same\n");
> > goto fail;
> > }
> > --
> > 2.17.1
@@ -444,7 +444,12 @@ test_ring_burst_bulk_tests1(unsigned int test_idx)
TEST_RING_VERIFY(rte_ring_empty(r));
/* check data */
- TEST_RING_VERIFY(memcmp(src, dst, rsz) == 0);
+ if (esize[i] == -1) {
+ TEST_RING_VERIFY(memcmp(src, dst,
+ rsz * sizeof(void *)) == 0);
+ } else
+ TEST_RING_VERIFY(memcmp(src, dst,
+ rsz * esize[i]) == 0);
}
/* Free memory before test completed */
@@ -538,9 +543,11 @@ test_ring_burst_bulk_tests2(unsigned int test_idx)
cur_dst = test_ring_inc_ptr(cur_dst, esize[i], MAX_BULK);
/* check data */
- if (memcmp(src, dst, cur_dst - dst)) {
- rte_hexdump(stdout, "src", src, cur_src - src);
- rte_hexdump(stdout, "dst", dst, cur_dst - dst);
+ if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
+ rte_hexdump(stdout, "src", src,
+ RTE_PTR_DIFF(cur_src, src));
+ rte_hexdump(stdout, "dst", dst,
+ RTE_PTR_DIFF(cur_dst, dst));
printf("data after dequeue is not the same\n");
goto fail;
}
@@ -614,9 +621,11 @@ test_ring_burst_bulk_tests3(unsigned int test_idx)
}
/* check data */
- if (memcmp(src, dst, cur_dst - dst)) {
- rte_hexdump(stdout, "src", src, cur_src - src);
- rte_hexdump(stdout, "dst", dst, cur_dst - dst);
+ if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
+ rte_hexdump(stdout, "src", src,
+ RTE_PTR_DIFF(cur_src, src));
+ rte_hexdump(stdout, "dst", dst,
+ RTE_PTR_DIFF(cur_dst, dst));
printf("data after dequeue is not the same\n");
goto fail;
}
@@ -747,9 +756,11 @@ test_ring_burst_bulk_tests4(unsigned int test_idx)
goto fail;
/* check data */
- if (memcmp(src, dst, cur_dst - dst)) {
- rte_hexdump(stdout, "src", src, cur_src - src);
- rte_hexdump(stdout, "dst", dst, cur_dst - dst);
+ if (memcmp(src, dst, RTE_PTR_DIFF(cur_dst, dst))) {
+ rte_hexdump(stdout, "src", src,
+ RTE_PTR_DIFF(cur_src, src));
+ rte_hexdump(stdout, "dst", dst,
+ RTE_PTR_DIFF(cur_dst, dst));
printf("data after dequeue is not the same\n");
goto fail;
}