[dpdk-dev] [PATCH v3] test: fix missing NULL pointer checks

[dpdk-dev] [PATCH v3] test: fix missing NULL pointer checks

[Daniel Mrzyglod]

In test_sched, we are missing NULL pointer checks after create_mempool()
and rte_pktmbuf_alloc(). Add in these checks using TEST_ASSERT_NOT_NULL macros.

VERIFY macro was removed and replaced by standard test ASSERTS from "test.h" header.
This provides additional information to track when the failure occured.

v3 changes:
- remove VERIFY macro
- fix spelling error.
- change unproper comment

v2 changes:
- Replace all VERIFY macros instances by proper TEST_ASSERT* macros.
- fix description

v1 changes:
- first iteration of patch using VERIFY macro.

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 app/test/test_sched.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/app/test/test_sched.c b/app/test/test_sched.c
index c957d80..60c62de 100644
--- a/app/test/test_sched.c
+++ b/app/test/test_sched.c
@@ -46,13 +46,6 @@
 #include <rte_sched.h>
 
 
-#define VERIFY(exp,fmt,args...)                    	                \
-		if (!(exp)) {                                               \
-			printf(fmt, ##args);                                    \
-			return -1;                                              \
-		}
-
-
 #define SUBPORT 	0
 #define PIPE 		1
 #define TC 			2
@@ -166,48 +159,49 @@ test_sched(void)
 	int err;
 
 	mp = create_mempool();
+	TEST_ASSERT_NOT_NULL(mp, "Error creating mempool\n");
 
 	port_param.socket = 0;
 	port_param.rate = (uint64_t) 10000 * 1000 * 1000 / 8;
 
 	port = rte_sched_port_config(&port_param);
-	VERIFY(port != NULL, "Error config sched port\n");
-
+	TEST_ASSERT_NOT_NULL(port, "Error config sched port\n");
 
 	err = rte_sched_subport_config(port, SUBPORT, subport_param);
-	VERIFY(err == 0, "Error config sched, err=%d\n", err);
+	TEST_ASSERT_SUCCESS(err, "Error config sched, err=%d\n", err);
 
 	for (pipe = 0; pipe < port_param.n_pipes_per_subport; pipe ++) {
 		err = rte_sched_pipe_config(port, SUBPORT, pipe, 0);
-		VERIFY(err == 0, "Error config sched pipe %u, err=%d\n", pipe, err);
+		TEST_ASSERT_SUCCESS(err, "Error config sched pipe %u, err=%d\n", pipe, err);
 	}
 
 	for (i = 0; i < 10; i++) {
 		in_mbufs[i] = rte_pktmbuf_alloc(mp);
+		TEST_ASSERT_NOT_NULL(in_mbufs[i], "Packet allocation failed\n");
 		prepare_pkt(in_mbufs[i]);
 	}
 
 
 	err = rte_sched_port_enqueue(port, in_mbufs, 10);
-	VERIFY(err == 10, "Wrong enqueue, err=%d\n", err);
+	TEST_ASSERT_EQUAL(err, 10, "Wrong enqueue, err=%d\n", err);
 
 	err = rte_sched_port_dequeue(port, out_mbufs, 10);
-	VERIFY(err == 10, "Wrong dequeue, err=%d\n", err);
+	TEST_ASSERT_EQUAL(err, 10, "Wrong dequeue, err=%d\n", err);
 
 	for (i = 0; i < 10; i++) {
 		enum rte_meter_color color;
 		uint32_t subport, traffic_class, queue;
 
 		color = rte_sched_port_pkt_read_color(out_mbufs[i]);
-		VERIFY(color == e_RTE_METER_YELLOW, "Wrong color\n");
+		TEST_ASSERT_EQUAL(color, e_RTE_METER_YELLOW, "Wrong color\n");
 
 		rte_sched_port_pkt_read_tree_path(out_mbufs[i],
 				&subport, &pipe, &traffic_class, &queue);
 
-		VERIFY(subport == SUBPORT, "Wrong subport\n");
-		VERIFY(pipe == PIPE, "Wrong pipe\n");
-		VERIFY(traffic_class == TC, "Wrong traffic_class\n");
-		VERIFY(queue == QUEUE, "Wrong queue\n");
+		TEST_ASSERT_EQUAL(subport, SUBPORT, "Wrong subport\n");
+		TEST_ASSERT_EQUAL(pipe, PIPE, "Wrong pipe\n");
+		TEST_ASSERT_EQUAL(traffic_class, TC, "Wrong traffic_class\n");
+		TEST_ASSERT_EQUAL(queue, QUEUE, "Wrong queue\n");
 
 	}
 
@@ -215,12 +209,15 @@ test_sched(void)
 	struct rte_sched_subport_stats subport_stats;
 	uint32_t tc_ov;
 	rte_sched_subport_read_stats(port, SUBPORT, &subport_stats, &tc_ov);
-	//VERIFY(subport_stats.n_pkts_tc[TC-1] == 10, "Wrong subport stats\n");
-
+#if 0
+	TEST_ASSERT_EQUAL(subport_stats.n_pkts_tc[TC-1], 10, "Wrong subport stats\n");
+#endif
 	struct rte_sched_queue_stats queue_stats;
 	uint16_t qlen;
 	rte_sched_queue_read_stats(port, QUEUE, &queue_stats, &qlen);
-	//VERIFY(queue_stats.n_pkts == 10, "Wrong queue stats\n");
+#if 0
+	TEST_ASSERT_EQUAL(queue_stats.n_pkts, 10, "Wrong queue stats\n");
+#endif
 
 	rte_sched_port_free(port);
 
-- 
2.1.0

Re: [dpdk-dev] [PATCH v3] test: fix missing NULL pointer checks

[Neil Horman]

On Tue, Jan 27, 2015 at 04:44:53PM +0100, Daniel Mrzyglod wrote:
> In test_sched, we are missing NULL pointer checks after create_mempool()
> and rte_pktmbuf_alloc(). Add in these checks using TEST_ASSERT_NOT_NULL macros.
> 
> VERIFY macro was removed and replaced by standard test ASSERTS from "test.h" header.
> This provides additional information to track when the failure occured.
> 
> v3 changes:
> - remove VERIFY macro
> - fix spelling error.
> - change unproper comment
> 
> v2 changes:
> - Replace all VERIFY macros instances by proper TEST_ASSERT* macros.
> - fix description
> 
> v1 changes:
> - first iteration of patch using VERIFY macro.
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
>  app/test/test_sched.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/app/test/test_sched.c b/app/test/test_sched.c
> index c957d80..60c62de 100644
> --- a/app/test/test_sched.c
> +++ b/app/test/test_sched.c
> @@ -46,13 +46,6 @@
>  #include <rte_sched.h>
>  
>  
> -#define VERIFY(exp,fmt,args...)                    	                \
> -		if (!(exp)) {                                               \
> -			printf(fmt, ##args);                                    \
> -			return -1;                                              \
> -		}
> -
> -
>  #define SUBPORT 	0
>  #define PIPE 		1
>  #define TC 			2
> @@ -166,48 +159,49 @@ test_sched(void)
>  	int err;
>  
>  	mp = create_mempool();
> +	TEST_ASSERT_NOT_NULL(mp, "Error creating mempool\n");
>  
>  	port_param.socket = 0;
>  	port_param.rate = (uint64_t) 10000 * 1000 * 1000 / 8;
>  
>  	port = rte_sched_port_config(&port_param);
> -	VERIFY(port != NULL, "Error config sched port\n");
> -
> +	TEST_ASSERT_NOT_NULL(port, "Error config sched port\n");
>  
>  	err = rte_sched_subport_config(port, SUBPORT, subport_param);
> -	VERIFY(err == 0, "Error config sched, err=%d\n", err);
> +	TEST_ASSERT_SUCCESS(err, "Error config sched, err=%d\n", err);
>  
>  	for (pipe = 0; pipe < port_param.n_pipes_per_subport; pipe ++) {
>  		err = rte_sched_pipe_config(port, SUBPORT, pipe, 0);
> -		VERIFY(err == 0, "Error config sched pipe %u, err=%d\n", pipe, err);
> +		TEST_ASSERT_SUCCESS(err, "Error config sched pipe %u, err=%d\n", pipe, err);
>  	}
>  
>  	for (i = 0; i < 10; i++) {
>  		in_mbufs[i] = rte_pktmbuf_alloc(mp);
> +		TEST_ASSERT_NOT_NULL(in_mbufs[i], "Packet allocation failed\n");
>  		prepare_pkt(in_mbufs[i]);
>  	}
>  
>  
>  	err = rte_sched_port_enqueue(port, in_mbufs, 10);
> -	VERIFY(err == 10, "Wrong enqueue, err=%d\n", err);
> +	TEST_ASSERT_EQUAL(err, 10, "Wrong enqueue, err=%d\n", err);
>  
>  	err = rte_sched_port_dequeue(port, out_mbufs, 10);
> -	VERIFY(err == 10, "Wrong dequeue, err=%d\n", err);
> +	TEST_ASSERT_EQUAL(err, 10, "Wrong dequeue, err=%d\n", err);
>  
>  	for (i = 0; i < 10; i++) {
>  		enum rte_meter_color color;
>  		uint32_t subport, traffic_class, queue;
>  
>  		color = rte_sched_port_pkt_read_color(out_mbufs[i]);
> -		VERIFY(color == e_RTE_METER_YELLOW, "Wrong color\n");
> +		TEST_ASSERT_EQUAL(color, e_RTE_METER_YELLOW, "Wrong color\n");
>  
>  		rte_sched_port_pkt_read_tree_path(out_mbufs[i],
>  				&subport, &pipe, &traffic_class, &queue);
>  
> -		VERIFY(subport == SUBPORT, "Wrong subport\n");
> -		VERIFY(pipe == PIPE, "Wrong pipe\n");
> -		VERIFY(traffic_class == TC, "Wrong traffic_class\n");
> -		VERIFY(queue == QUEUE, "Wrong queue\n");
> +		TEST_ASSERT_EQUAL(subport, SUBPORT, "Wrong subport\n");
> +		TEST_ASSERT_EQUAL(pipe, PIPE, "Wrong pipe\n");
> +		TEST_ASSERT_EQUAL(traffic_class, TC, "Wrong traffic_class\n");
> +		TEST_ASSERT_EQUAL(queue, QUEUE, "Wrong queue\n");
>  
>  	}
>  
> @@ -215,12 +209,15 @@ test_sched(void)
>  	struct rte_sched_subport_stats subport_stats;
>  	uint32_t tc_ov;
>  	rte_sched_subport_read_stats(port, SUBPORT, &subport_stats, &tc_ov);
> -	//VERIFY(subport_stats.n_pkts_tc[TC-1] == 10, "Wrong subport stats\n");
> -
> +#if 0
> +	TEST_ASSERT_EQUAL(subport_stats.n_pkts_tc[TC-1], 10, "Wrong subport stats\n");
> +#endif
>  	struct rte_sched_queue_stats queue_stats;
>  	uint16_t qlen;
>  	rte_sched_queue_read_stats(port, QUEUE, &queue_stats, &qlen);
> -	//VERIFY(queue_stats.n_pkts == 10, "Wrong queue stats\n");
> +#if 0
> +	TEST_ASSERT_EQUAL(queue_stats.n_pkts, 10, "Wrong queue stats\n");
> +#endif
>  
>  	rte_sched_port_free(port);
>  
> -- 
> 2.1.0
> 
> 
These TEST_ASSERT macros are no better than the VERIFY macro, they contain
exaxtly the same return issue that I outlined in my first post on the subject.
Neil

Re: [dpdk-dev] [PATCH v3] test: fix missing NULL pointer checks

[Thomas Monjalon]

2015-01-27 13:06, Neil Horman:
> On Tue, Jan 27, 2015 at 04:44:53PM +0100, Daniel Mrzyglod wrote:
> > In test_sched, we are missing NULL pointer checks after create_mempool()
> > and rte_pktmbuf_alloc(). Add in these checks using TEST_ASSERT_NOT_NULL macros.
> > 
> > VERIFY macro was removed and replaced by standard test ASSERTS from "test.h" header.
> > This provides additional information to track when the failure occured.
> > 
> > v3 changes:
> > - remove VERIFY macro
> > - fix spelling error.
> > - change unproper comment
> > 
> > v2 changes:
> > - Replace all VERIFY macros instances by proper TEST_ASSERT* macros.
> > - fix description
> > 
> > v1 changes:
> > - first iteration of patch using VERIFY macro.
> > 
> > Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
>   
> These TEST_ASSERT macros are no better than the VERIFY macro, they contain
> exaxtly the same return issue that I outlined in my first post on the subject.

Neil, you are suggesting to rework the assert macros of the unit tests.
It should be another patch.
Here, Daniel is improving the sched test with existing macros.
I think it should be applied.

-- 
Thomas

Re: [dpdk-dev] [PATCH v3] test: fix missing NULL pointer checks

[Bruce Richardson]

On Fri, Jan 30, 2015 at 11:18:19AM +0100, Thomas Monjalon wrote:
> 2015-01-27 13:06, Neil Horman:
> > On Tue, Jan 27, 2015 at 04:44:53PM +0100, Daniel Mrzyglod wrote:
> > > In test_sched, we are missing NULL pointer checks after create_mempool()
> > > and rte_pktmbuf_alloc(). Add in these checks using TEST_ASSERT_NOT_NULL macros.
> > > 
> > > VERIFY macro was removed and replaced by standard test ASSERTS from "test.h" header.
> > > This provides additional information to track when the failure occured.
> > > 
> > > v3 changes:
> > > - remove VERIFY macro
> > > - fix spelling error.
> > > - change unproper comment
> > > 
> > > v2 changes:
> > > - Replace all VERIFY macros instances by proper TEST_ASSERT* macros.
> > > - fix description
> > > 
> > > v1 changes:
> > > - first iteration of patch using VERIFY macro.
> > > 
> > > Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> >   
> > These TEST_ASSERT macros are no better than the VERIFY macro, they contain
> > exaxtly the same return issue that I outlined in my first post on the subject.
> 
> Neil, you are suggesting to rework the assert macros of the unit tests.
> It should be another patch.
> Here, Daniel is improving the sched test with existing macros.
> I think it should be applied.
>

+1
I agree with Thomas here. Having looked at the V4 patch, I believe this V3 is
better, and that other cleanup should be done in separate patches.

/Bruce

Re: [dpdk-dev] [PATCH v3] test: fix missing NULL pointer checks

[Thomas Monjalon]

2015-02-10 11:46, Bruce Richardson:
> On Fri, Jan 30, 2015 at 11:18:19AM +0100, Thomas Monjalon wrote:
> > 2015-01-27 13:06, Neil Horman:
> > > On Tue, Jan 27, 2015 at 04:44:53PM +0100, Daniel Mrzyglod wrote:
> > > > In test_sched, we are missing NULL pointer checks after create_mempool()
> > > > and rte_pktmbuf_alloc(). Add in these checks using TEST_ASSERT_NOT_NULL macros.
> > > > 
> > > > VERIFY macro was removed and replaced by standard test ASSERTS from "test.h" header.
> > > > This provides additional information to track when the failure occured.
> > > > 
> > > > v3 changes:
> > > > - remove VERIFY macro
> > > > - fix spelling error.
> > > > - change unproper comment
> > > > 
> > > > v2 changes:
> > > > - Replace all VERIFY macros instances by proper TEST_ASSERT* macros.
> > > > - fix description
> > > > 
> > > > v1 changes:
> > > > - first iteration of patch using VERIFY macro.
> > > > 
> > > > Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> > >   
> > > These TEST_ASSERT macros are no better than the VERIFY macro, they contain
> > > exaxtly the same return issue that I outlined in my first post on the subject.
> > 
> > Neil, you are suggesting to rework the assert macros of the unit tests.
> > It should be another patch.
> > Here, Daniel is improving the sched test with existing macros.
> > I think it should be applied.
> >
> 
> +1
> I agree with Thomas here. Having looked at the V4 patch, I believe this V3 is
> better, and that other cleanup should be done in separate patches.

Applied, thanks