diff mbox

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

Message ID 1505228764-9738-2-git-send-email-pbhagavatula@caviumnetworks.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Pavan Nikhilesh Sept. 12, 2017, 3:06 p.m. UTC 
  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(-)

Comments

Bruce Richardson Oct. 12, 2017, 8:28 a.m. UTC | #1 
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
diff mbox

Patch

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;
 }
@@ -88,8 +90,8 @@  test_reorder_init(void)
 	unsigned int size;
 	/*
 	 * The minimum memory area size that should be passed to library is,
-	 * sizeof(struct rte_reorder_buffer) + (2 * size * sizeof(struct rte_mbuf *));
-	 * Otherwise error will be thrown
+	 * sizeof(struct rte_reorder_buffer) + (2 * size *
+	 * sizeof(struct rte_mbuf *)); Otherwise error will be thrown
 	 */
 
 	size = 100;
@@ -189,15 +191,16 @@  test_reorder_insert(void)
 	for (i = 0; i < size; i++) {
 		ret = rte_reorder_insert(b, bufs[i]);
 		if (ret != 0) {
-			printf("%s:%d: Error inserting packet with seqn less than size\n",
+			printf("%s:%d: Error inserting packet with seqn less"
+					" than size\n",
 					__func__, __LINE__);
 			ret = -1;
 			goto exit;
 		}
 	}
 
-	/* early packet - should move mbufs to ready buf and move sequence window
-	 * reorder_seq = 4
+	/* early packet - should move mbufs to ready buf and move sequence
+	 * window reorder_seq = 4
 	 * RB[] = {0, 1, 2, 3}
 	 * OB[] = {4, NULL, NULL, NULL}
 	 */
@@ -208,12 +211,14 @@  test_reorder_insert(void)
 		ret = -1;
 		goto exit;
 	}
+	i++;
 
 	/* early packet from current sequence window - full ready buffer */
 	bufs[5]->seqn = 2 * size;
 	ret = rte_reorder_insert(b, bufs[5]);
 	if (!((ret == -1) && (rte_errno == ENOSPC))) {
-		printf("%s:%d: No error inserting early packet with full ready buffer\n",
+		printf("%s:%d: No error inserting early packet with full ready"
+				" buffer\n",
 				__func__, __LINE__);
 		ret = -1;
 		goto exit;
@@ -231,7 +236,7 @@  test_reorder_insert(void)
 
 	ret = 0;
 exit:
-	rte_mempool_put_bulk(p, (void *)bufs, num_bufs);
+	rte_mempool_put_bulk(p, (void **)&bufs[i], num_bufs - i);
 	rte_reorder_free(b);
 	return ret;
 }
@@ -360,6 +365,13 @@  test_setup(void)
 	return 0;
 }
 
+static void
+test_destroy(void)
+{
+	rte_mempool_free(test_params->p);
+	test_params->p = NULL;
+}
+
 static struct unit_test_suite reorder_test_suite  = {
 
 	.setup = test_setup,
@@ -372,7 +384,8 @@  static struct unit_test_suite reorder_test_suite  = {
 		TEST_CASE(test_reorder_insert),
 		TEST_CASE(test_reorder_drain),
 		TEST_CASES_END()
-	}
+	},
+	.teardown = test_destroy
 };
 
 static int