Hi Marko,
> -----Original Message-----
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Marko Kovacevic
> Sent: Thursday, December 20, 2018 2:58 PM
> To: dev@dpdk.org
> Cc: akhil.go...@nxp.com; Daly, Lee <lee.d...@intel.com>; Jozwiak, TomaszX
> <tomaszx.jozw...@intel.com>; O'Hare, Cathal <cathal.oh...@intel.com>;
> Trahe, Fiona <fiona.tr...@intel.com>; Kovacevic, Marko
> <marko.kovace...@intel.com>
> Subject: [dpdk-dev] [PATCH v2 1/2] test/compress: add out of space test
>
> From: "Kovacevic, Marko" <marko.kovace...@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.kovace...@intel.com>
> Acked-by: Lee Daly <lee.d...@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.
