[v2,1/2] test/compress: add out of space test

  From: "Kovacevic, Marko" <marko.kovacevic@intel.com>

This patch adds new out of space testcase to check
that the destination mbuf is smaller than required for
the output of compression to ensure the driver doesn't crash
and returns the valid error case.

Signed-off-by: Marko Kovacevic <marko.kovacevic@intel.com>
Acked-by: Lee Daly <lee.daly@intel.com>

---
V2:
  Added check flag to return proper status to user
---
 test/test/test_compressdev.c | 130 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 126 insertions(+), 4 deletions(-)
  

> -----Original Message-----
> From: Kovacevic, Marko
> Sent: Thursday, December 20, 2018 7:58 AM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Daly, Lee <lee.daly@intel.com>; Jozwiak, TomaszX
> <tomaszx.jozwiak@intel.com>; O'Hare, Cathal <cathal.ohare@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>
> Subject: [PATCH v2 1/2] test/compress: add out of space test
> 
> From: "Kovacevic, Marko" <marko.kovacevic@intel.com>
> 
> This patch adds new out of space testcase to check
> that the destination mbuf is smaller than required for
> the output of compression to ensure the driver doesn't crash
> and returns the valid error case.
> 
> Signed-off-by: Marko Kovacevic <marko.kovacevic@intel.com>
> Acked-by: Lee Daly <lee.daly@intel.com>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>
  

Hi Marko,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Marko Kovacevic
> Sent: Thursday, December 20, 2018 2:58 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Daly, Lee <lee.daly@intel.com>; Jozwiak, TomaszX
> <tomaszx.jozwiak@intel.com>; O'Hare, Cathal <cathal.ohare@intel.com>;
> Trahe, Fiona <fiona.trahe@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>
> Subject: [dpdk-dev] [PATCH v2 1/2] test/compress: add out of space test
> 
> From: "Kovacevic, Marko" <marko.kovacevic@intel.com>
> 
> This patch adds new out of space testcase to check that the destination
> mbuf is smaller than required for the output of compression to ensure the
> driver doesn't crash and returns the valid error case.
> 
> Signed-off-by: Marko Kovacevic <marko.kovacevic@intel.com>
> Acked-by: Lee Daly <lee.daly@intel.com>
> 
> ---
> V2:
>   Added check flag to return proper status to user
> ---
>  test/test/test_compressdev.c | 130
> +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 126 insertions(+), 4 deletions(-)
> 
> diff --git a/test/test/test_compressdev.c b/test/test/test_compressdev.c
> index 42327dc..b2999fa 100644
> --- a/test/test/test_compressdev.c
> +++ b/test/test/test_compressdev.c
> @@ -41,6 +41,9 @@
>  #define ZLIB_TRAILER_SIZE 4
>  #define GZIP_HEADER_SIZE 10
>  #define GZIP_TRAILER_SIZE 8
> +#define OUT_OF_SPACE_BUF 1
> +
> +int out_of_space;

I think we should avoid global variables if possible.
This is a variable to change a test type, so it should go with the other test parameters.
I get that the list of arguments is growing a lot, therefore refactoring
the test_deflate_comp_decomp function is highly needed.
I will send a patch doing that, so code is cleaner.
> 
>  const char *
>  huffman_type_strings[] = {
> @@ -734,8 +737,9 @@ test_deflate_comp_decomp(const char * const
> test_bufs[],
> 
>  	if (sgl) {
>  		for (i = 0; i < num_bufs; i++) {
> -			data_size = strlen(test_bufs[i]) *
> -				COMPRESS_BUF_SIZE_RATIO;
> +			out_of_space ? data_size = OUT_OF_SPACE_BUF :
> +					(data_size = strlen(test_bufs[i]) *
> +					COMPRESS_BUF_SIZE_RATIO);

This is OK when compressing with compressdev.
When testing out of place error when decompressing with compressdev,
then compression (with Zlib) needs to be successful.
Therefore, data_size should only be equal to OUT_OF_SPACE_BUF
when zlib_dir = ZLIB_COMPRESS and out_of_space = 1.

>  			if (prepare_sgl_bufs(NULL, comp_bufs[i],
>  					data_size,
>  					ts_params->small_mbuf_pool,
> @@ -746,8 +750,9 @@ test_deflate_comp_decomp(const char * const
> test_bufs[],
> 
>  	} else {
>  		for (i = 0; i < num_bufs; i++) {
> -			data_size = strlen(test_bufs[i]) *
> -				COMPRESS_BUF_SIZE_RATIO;
> +			out_of_space ? data_size = OUT_OF_SPACE_BUF :
> +					(data_size = strlen(test_bufs[i]) *
> +					COMPRESS_BUF_SIZE_RATIO);
>  			rte_pktmbuf_append(comp_bufs[i], data_size);
>  		}
>  	}
> @@ -913,6 +918,16 @@ test_deflate_comp_decomp(const char * const
> test_bufs[],
>  	 * compress operation information is needed for the decompression
> stage)
>  	 */
>  	for (i = 0; i < num_bufs; i++) {
> +		if(out_of_space) {
> +			if (ops_processed[i]->status ==
> +
> 	RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED) {
> +				RTE_LOG(ERR, USER1,
> +				"Some operations were not successful\n");

I think this log should be removed, as if status is "out of space terminated",
the test is actually passing (this is a negative test), so there is no error.
Instead, the log should be placed in the else part of this condition,
with a message saying that the status is incorrect.

> +				out_of_space = 0;

I think instead of using out_of_space as an output, ret_status can be changed to 0
and just exit the function successfully.
If status is not the expected, then ret_status = -1 (default, so no need to change).
Also, all operations should be checked if their status is the right one.
Therefore, one thing you can do is check if not status = OOS_TERMINATED and return an error in that case.
Then, after the loop, if out_of_space = 1, just go to exit.

> +				goto exit;
> +			}
> +		}
> +
>  		if (ops_processed[i]->status !=
> RTE_COMP_OP_STATUS_SUCCESS) {
>  			RTE_LOG(ERR, USER1,
>  				"Some operations were not successful\n");
> @@ -1110,6 +1125,16 @@ test_deflate_comp_decomp(const char * const
> test_bufs[],
>  	 * compress operation information is still needed)
>  	 */
>  	for (i = 0; i < num_bufs; i++) {
> +		if(out_of_space) {
> +			if (ops_processed[i]->status ==
> +
> 	RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED) {
> +				RTE_LOG(ERR, USER1,
> +				"Some operations were not successful\n");
> +				out_of_space = 0;
> +				goto exit;

Same comments as above apply here.
Also, data_size of the output buffers need to be modified as it is done for compression.

> +			}
> +		}
> +
>  		if (ops_processed[i]->status !=
> RTE_COMP_OP_STATUS_SUCCESS) {
>  			RTE_LOG(ERR, USER1,
>  				"Some operations were not successful\n");
> @@ -1664,6 +1689,101 @@
> test_compressdev_deflate_stateless_checksum(void)
>  	return ret;
>  }
> 
> +static int
> +test_compressdev_out_of_space_buffer(void)
> +{
> +	struct comp_testsuite_params *ts_params = &testsuite_params;
> +	const char *test_buffer;
> +	int ret;
> +	uint16_t i = 0;
> +	const struct rte_compressdev_capabilities *capab;
> +	out_of_space = 1;
> +
> +	capab = rte_compressdev_capability_get(0,
> RTE_COMP_ALGO_DEFLATE);
> +	TEST_ASSERT(capab != NULL, "Failed to retrieve device capabilities");
> +
> +	if ((capab->comp_feature_flags & RTE_COMP_FF_HUFFMAN_FIXED)
> == 0)
> +		return -ENOTSUP;
> +
> +	struct rte_comp_xform *compress_xform =
> +			rte_malloc(NULL, sizeof(struct rte_comp_xform), 0);
> +
> +	if (compress_xform == NULL) {
> +		RTE_LOG(ERR, USER1,
> +			"Compress xform could not be created\n");
> +		ret = TEST_FAILED;
> +		goto exit;
> +	}
> +
> +	test_buffer = compress_test_bufs[i];
> +
> +	/* Compress with compressdev, decompress with Zlib */
> +	if (test_deflate_comp_decomp(&test_buffer, 1,
> +			&i,
> +			&ts_params->def_comp_xform,
> +			&ts_params->def_decomp_xform,
> +			1,
> +			RTE_COMP_OP_STATELESS,
> +			0,
> +			ZLIB_DECOMPRESS) == 0 && out_of_space == 1) {

As said above, I think out_of_space does not need to be checked as an ouput.
Instead, only the return value of this function should be checked.