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

  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(-)
  

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
  

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.
  

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
  

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