Re: [dpdk-dev] [PATCH 2/2] test/reorder: fix reorder drain test

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Sep 12, 2017 at 08:36:04PM +0530, Pavan Nikhilesh wrote:
> The reorder drain test fails due to mempool corruption caused by freeing
> packet buffer twice.
> 
> Fixes: d0c9b58d7156 ("app/test: new reorder unit test")
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> ---
>  test/test/test_reorder.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/test/test/test_reorder.c b/test/test/test_reorder.c
> index 4ec22ac..51c2dcd 100644
> --- a/test/test/test_reorder.c
> +++ b/test/test/test_reorder.c
> @@ -70,13 +70,15 @@ test_reorder_create(void)
>  	TEST_ASSERT((b == NULL) && (rte_errno == EINVAL),
>  			"No error on create() with NULL name");
>  
> -	b = rte_reorder_create("PKT", rte_socket_id(), REORDER_BUFFER_SIZE_INVALID);
> +	b = rte_reorder_create("PKT", rte_socket_id(),
> +			REORDER_BUFFER_SIZE_INVALID);
>  	TEST_ASSERT((b == NULL) && (rte_errno == EINVAL),
>  			"No error on create() with invalid buffer size param.");
>  
>  	b = rte_reorder_create("PKT_RO1", rte_socket_id(), REORDER_BUFFER_SIZE);
>  	TEST_ASSERT_EQUAL(b, test_params->b,
> -			"New reorder instance created with already existing name");
> +			"New reorder instance created with already existing"
> +			" name");
>  
>  	return 0;
>  }

These changes are just cosmetic and so shouldn't really be included in
this patch. Ideally, cosmetic changes should be made only when you are
already touching the affected lines anyway due to some other change.
Also, it's bad practice to split literal strings across lines, even if
the line ends up crossing the 80-char threshold. Being able to grep
error strings is more important than keeping lines short.

This applies to many of the other changes in this patch. It makes it hard to
review for the actual functional changes.

Regards,
/Bruce