test/compress: fix max mbuf size test case
Checks
Commit Message
Fixed the compilation error on gcc (GCC)
4.8.5 20150623 (Red Hat 4.8.5-28)
Fixes: 355b02eedc65 ("test/compress: add max mbuf size test case")
Cc: stable@dpdk.org
Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
---
app/test/test_compressdev.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Comments
On Fri, Apr 19, 2019 at 11:57:07AM +0200, Tomasz Jozwiak wrote:
> Fixed the compilation error on gcc (GCC)
> 4.8.5 20150623 (Red Hat 4.8.5-28)
>
> Fixes: 355b02eedc65 ("test/compress: add max mbuf size test case")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> ---
> app/test/test_compressdev.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
This fixes compilation for me with gcc4.8, though I can't comment on the
correctness of the code.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Please check my comments below:
19/04/2019 11:57, Tomasz Jozwiak:
> Fixed the compilation error on gcc (GCC)
> 4.8.5 20150623 (Red Hat 4.8.5-28)
It is seen with more compilers:
https://build.opensuse.org/project/show/home:bluca:dpdk
Please add the log of the error.
> Fixes: 355b02eedc65 ("test/compress: add max mbuf size test case")
> Cc: stable@dpdk.org
It is a recent commit, no need to Cc stable for backport.
> Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> ---
> app/test/test_compressdev.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_compressdev.c b/app/test/test_compressdev.c
> index 404b98f..603eeea 100644
> --- a/app/test/test_compressdev.c
> +++ b/app/test/test_compressdev.c
> @@ -1948,6 +1948,7 @@ test_compressdev_deflate_stateless_dynamic_big(void)
> struct comp_testsuite_params *ts_params = &testsuite_params;
> uint16_t i = 0;
> int ret = TEST_SUCCESS;
> + int j = 0;
> const struct rte_compressdev_capabilities *capab;
> char *test_buffer = NULL;
>
> @@ -1989,8 +1990,8 @@ test_compressdev_deflate_stateless_dynamic_big(void)
>
> /* fill the buffer with data based on rand. data */
> srand(BIG_DATA_TEST_SIZE);
> - for (uint32_t i = 0; i < BIG_DATA_TEST_SIZE - 1; ++i)
> - test_buffer[i] = (uint8_t)(rand() % ((uint8_t)-1)) | 1;
> + for (j = 0; j < BIG_DATA_TEST_SIZE - 1; ++j)
> + test_buffer[j] = (uint8_t)(rand() % ((uint8_t)-1)) | 1;
>
> test_buffer[BIG_DATA_TEST_SIZE-1] = 0;
> int_data.buf_idx = &i;
What is supposed to be "i"?
It is initialized at 0 and never touched.
On Fri, Apr 19, 2019 at 11:57 AM Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
wrote:
> Fixed the compilation error on gcc (GCC)
> 4.8.5 20150623 (Red Hat 4.8.5-28)
>
> Fixes: 355b02eedc65 ("test/compress: add max mbuf size test case")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> ---
> app/test/test_compressdev.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_compressdev.c b/app/test/test_compressdev.c
> index 404b98f..603eeea 100644
> --- a/app/test/test_compressdev.c
> +++ b/app/test/test_compressdev.c
> @@ -1948,6 +1948,7 @@ test_compressdev_deflate_stateless_dynamic_big(void)
> struct comp_testsuite_params *ts_params = &testsuite_params;
> uint16_t i = 0;
> int ret = TEST_SUCCESS;
> + int j = 0;
>
Useless initialisation.
const struct rte_compressdev_capabilities *capab;
> char *test_buffer = NULL;
>
> @@ -1989,8 +1990,8 @@ test_compressdev_deflate_stateless_dynamic_big(void)
>
> /* fill the buffer with data based on rand. data */
> srand(BIG_DATA_TEST_SIZE);
> - for (uint32_t i = 0; i < BIG_DATA_TEST_SIZE - 1; ++i)
> - test_buffer[i] = (uint8_t)(rand() % ((uint8_t)-1)) | 1;
> + for (j = 0; j < BIG_DATA_TEST_SIZE - 1; ++j)
> + test_buffer[j] = (uint8_t)(rand() % ((uint8_t)-1)) | 1;
>
> test_buffer[BIG_DATA_TEST_SIZE-1] = 0;
> int_data.buf_idx = &i;
> --
> 2.7.4
>
Tested-by: David Marchand <david.marchand@redhat.com>
19/04/2019 12:57, Jozwiak, TomaszX:
> > What is supposed to be "i"?
> > It is initialized at 0 and never touched.
> >
> It's touched inside test_deflate_comp_decomp function.
What do you mean?
It's a local variable and its address is referenced:
int_data.buf_idx = &i;
It looks really wrong.
Another error is seen in FreeBSD.
Should I totally revert this patch?
test_deflate_comp_decomp function is common for all test cases.
The options for this function are inside struct interim_data_params,
which is passed to test_deflate_comp_decomp function as a pointer.
The field buf_idx should be initialized because is used inside test_deflate_comp_decomp
That's the reason of:
int_data.buf_idx = &i;
I'm not an author of this solution - sorry.
We can review this and try to add new solution.
Thx, Tomek
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, April 19, 2019 1:26 PM
> To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> david.marchand@redhat.com; Trahe, Fiona <fiona.trahe@intel.com>;
> yskoh@mellanox.com; Cel, TomaszX <tomaszx.cel@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] test/compress: fix max mbuf size test case
>
> 19/04/2019 12:57, Jozwiak, TomaszX:
> > > What is supposed to be "i"?
> > > It is initialized at 0 and never touched.
> > >
> > It's touched inside test_deflate_comp_decomp function.
>
> What do you mean?
> It's a local variable and its address is referenced:
> int_data.buf_idx = &i;
> It looks really wrong.
>
> Another error is seen in FreeBSD.
>
> Should I totally revert this patch?
>
>
Please stop top-posting, and read again below:
19/04/2019 13:36, Jozwiak, TomaszX:
> test_deflate_comp_decomp function is common for all test cases.
> The options for this function are inside struct interim_data_params,
> which is passed to test_deflate_comp_decomp function as a pointer.
>
> The field buf_idx should be initialized because is used inside test_deflate_comp_decomp
> That's the reason of:
> int_data.buf_idx = &i;
>
> I'm not an author of this solution - sorry.
>
> We can review this and try to add new solution.
I am not talking about int_data but the use of "i".
Please look again your patch, you are using "j" instead of "i"
and "i" is kept alone.
Moreover I don't know how the pointer of a local variable can be used.
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 19/04/2019 12:57, Jozwiak, TomaszX:
> > > > What is supposed to be "i"?
> > > > It is initialized at 0 and never touched.
> > > >
> > > It's touched inside test_deflate_comp_decomp function.
> >
> > What do you mean?
> > It's a local variable and its address is referenced:
> > int_data.buf_idx = &i;
> > It looks really wrong.
> >
> > Another error is seen in FreeBSD.
> >
> > Should I totally revert this patch?
> Moreover I don't know how the pointer of a local variable can be used.
That usage is in all test cases in this file - so probably should be fixed first.
May be I'm not an expert but what's wrong with usage of a pointer of local variable inside the function?
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, April 19, 2019 1:50 PM
> To: Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>;
> david.marchand@redhat.com; Trahe, Fiona <fiona.trahe@intel.com>;
> yskoh@mellanox.com; Cel, TomaszX <tomaszx.cel@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] test/compress: fix max mbuf size test case
>
> Please stop top-posting, and read again below:
>
> 19/04/2019 13:36, Jozwiak, TomaszX:
> > test_deflate_comp_decomp function is common for all test cases.
> > The options for this function are inside struct interim_data_params,
> > which is passed to test_deflate_comp_decomp function as a pointer.
> >
> > The field buf_idx should be initialized because is used inside
> > test_deflate_comp_decomp That's the reason of:
> > int_data.buf_idx = &i;
> >
> > I'm not an author of this solution - sorry.
> >
> > We can review this and try to add new solution.
>
> I am not talking about int_data but the use of "i".
> Please look again your patch, you are using "j" instead of "i"
> and "i" is kept alone.
> Moreover I don't know how the pointer of a local variable can be used.
>
>
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 19/04/2019 12:57, Jozwiak, TomaszX:
> > > > > What is supposed to be "i"?
> > > > > It is initialized at 0 and never touched.
> > > > >
> > > > It's touched inside test_deflate_comp_decomp function.
> > >
> > > What do you mean?
> > > It's a local variable and its address is referenced:
> > > int_data.buf_idx = &i;
> > > It looks really wrong.
> > >
> > > Another error is seen in FreeBSD.
> > >
> > > Should I totally revert this patch?
>
>
Hi Thomas et al,
> -----Original Message-----
> From: Jozwiak, TomaszX
> Sent: Friday, April 19, 2019 1:14 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; david.marchand@redhat.com;
> Trahe, Fiona <fiona.trahe@intel.com>; yskoh@mellanox.com; Cel, TomaszX <tomaszx.cel@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] test/compress: fix max mbuf size test case
[Fiona] Sorry for all the hassle. I've reviewed the related code and fix.
In my opinion the original fix was ok to resolve the compile issue, which
we missed due to using a different compiler version and also missed
spotting in code review. We'll be more careful in future.
I've sent a v3 with a small tweak to help the code clarity.
This is consistent with a bunch of other tests in same file.
@@ -1948,6 +1948,7 @@ test_compressdev_deflate_stateless_dynamic_big(void)
struct comp_testsuite_params *ts_params = &testsuite_params;
uint16_t i = 0;
int ret = TEST_SUCCESS;
+ int j = 0;
const struct rte_compressdev_capabilities *capab;
char *test_buffer = NULL;
@@ -1989,8 +1990,8 @@ test_compressdev_deflate_stateless_dynamic_big(void)
/* fill the buffer with data based on rand. data */
srand(BIG_DATA_TEST_SIZE);
- for (uint32_t i = 0; i < BIG_DATA_TEST_SIZE - 1; ++i)
- test_buffer[i] = (uint8_t)(rand() % ((uint8_t)-1)) | 1;
+ for (j = 0; j < BIG_DATA_TEST_SIZE - 1; ++j)
+ test_buffer[j] = (uint8_t)(rand() % ((uint8_t)-1)) | 1;
test_buffer[BIG_DATA_TEST_SIZE-1] = 0;
int_data.buf_idx = &i;