diff mbox

[dpdk-dev,3/6] test: fix memory leak in ring autotest

Message ID 5dee0afeaa0d007dfc9d949b1975c97537585ca4.1513867589.git.anatoly.burakov@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

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

Commit Message

Anatoly Burakov Dec. 22, 2017, 10:12 a.m. UTC 
  Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 test/test/test_ring.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Olivier Matz Dec. 22, 2017, 4:26 p.m. UTC | #1 
Hi,

On Fri, Dec 22, 2017 at 10:12:07AM +0000, Anatoly Burakov wrote:
> Fixes: af75078fece3 ("first public release")

Not sure about this commit id: freeing rings is only possible
since commit 4e32101f9b01 ("ring: support freeing").

[...]

> @@ -894,6 +895,8 @@ test_ring(void)
>  	/* dump the ring status */
>  	rte_ring_list_dump(stdout);
>  
> +	rte_ring_free(r);
> +
>  	return 0;

I think this is incorrect: r is a static variable, and if it is
not set to NULL, it will be reused at next call.

Ideally, removing the static variable would be better than just
resetting the value to NULL, but it will require more modifications:
add a ring argument to test function, and change return -1 -> goto fail.
Anatoly Burakov Dec. 23, 2017, 11:49 a.m. UTC | #2 
On 22-Dec-17 4:26 PM, Olivier MATZ wrote:
> Hi,
> 
> On Fri, Dec 22, 2017 at 10:12:07AM +0000, Anatoly Burakov wrote:
>> Fixes: af75078fece3 ("first public release")
> 
> Not sure about this commit id: freeing rings is only possible
> since commit 4e32101f9b01 ("ring: support freeing").

You're correct, i'll fix it in v2.

> 
> [...]
> 
>> @@ -894,6 +895,8 @@ test_ring(void)
>>   	/* dump the ring status */
>>   	rte_ring_list_dump(stdout);
>>   
>> +	rte_ring_free(r);
>> +
>>   	return 0;
> 
> I think this is incorrect: r is a static variable, and if it is
> not set to NULL, it will be reused at next call.

The point of these changes is not to leave any memory allocated after 
the test is done, regardless of whether it will be reused again.

> 
> Ideally, removing the static variable would be better than just
> resetting the value to NULL, but it will require more modifications:
> add a ring argument to test function, and change return -1 -> goto fail.
> 

...or rewrite it with the testsuite and do proper setup and teardown :) 
Anyway, i'll implement the suggestion in v2.
diff mbox

Patch

diff --git a/test/test/test_ring.c b/test/test/test_ring.c
index 5eb40a0..004d67e 100644
--- a/test/test/test_ring.c
+++ b/test/test/test_ring.c
@@ -760,6 +760,7 @@  test_ring_basic_ex(void)
 
 	ret = 0;
 fail_test:
+	rte_ring_free(rp);
 	if (obj != NULL)
 		rte_free(obj);
 
@@ -894,6 +895,8 @@  test_ring(void)
 	/* dump the ring status */
 	rte_ring_list_dump(stdout);
 
+	rte_ring_free(r);
+
 	return 0;
 }