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

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@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.