test/compress: fix max mbuf size test case

Message ID 1555667827-3715-1-git-send-email-tomaszx.jozwiak@intel.com
State Superseded, archived
Headers show
Series
  • test/compress: fix max mbuf size test case
Related show

Checks

Context Check Description
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Jozwiak, TomaszX April 19, 2019, 9:57 a.m.
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

Bruce Richardson April 19, 2019, 10:05 a.m. | #1
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>
Thomas Monjalon April 19, 2019, 10:17 a.m. | #2
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.
David Marchand April 19, 2019, 10:49 a.m. | #3
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>
Thomas Monjalon April 19, 2019, 11:25 a.m. | #4
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?
Jozwiak, TomaszX April 19, 2019, 11:36 a.m. | #5
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?
> 
>
Thomas Monjalon April 19, 2019, 11:50 a.m. | #6
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?
Jozwiak, TomaszX April 19, 2019, 12:14 p.m. | #7
> 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?
> 
>
Trahe, Fiona April 19, 2019, 5:01 p.m. | #8
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.

Patch

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;