[dpdk-dev] [PATCH] af_packet: Fix some klocwork errors

[dpdk-dev] [PATCH] af_packet: Fix some klocwork errors

[Ouyang Changchun]

Fix possible memory leak issue: free kvlist before return;
Fix possible resource lost issue: close qssockfd before return;

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf..cf8f4fa 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -694,6 +694,8 @@ error:
 		}
 		rte_free(*internals);
 	}
+	if (qsockfd != -1)
+		close(qsockfd);
 	return -1;
 }
 
@@ -822,16 +824,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
-		if (ret < 0)
+		if (ret < 0) {
+			rte_kvargs_free(kvlist);
 			return -1;
+		}
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
-	if (ret < 0)
+	if (ret < 0) {
+		rte_kvargs_free(kvlist);
 		return -1;
+	}
 
+	rte_kvargs_free(kvlist);
 	return 0;
 }
 
-- 
1.8.4.2

Re: [dpdk-dev] [PATCH] af_packet: Fix some klocwork errors

[Pawel Wodkowski]

On 2015-02-26 07:42, Ouyang Changchun wrote:
> Fix possible memory leak issue: free kvlist before return;
> Fix possible resource lost issue: close qssockfd before return;
>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
>   lib/librte_pmd_af_packet/rte_eth_af_packet.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 80e9bdf..cf8f4fa 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -694,6 +694,8 @@ error:
>   		}
>   		rte_free(*internals);
>   	}
> +	if (qsockfd != -1)
> +		close(qsockfd);
>   	return -1;
>   }
>
> @@ -822,16 +824,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>
>   		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
>   		                         &open_packet_iface, &sockfd);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			rte_kvargs_free(kvlist);
>   			return -1;
> +		}
>   	}
>
>   	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
>   	close(sockfd); /* no longer needed */
>
> -	if (ret < 0)
> +	if (ret < 0) {
> +		rte_kvargs_free(kvlist);
>   		return -1;
> +	}
>
> +	rte_kvargs_free(kvlist);
>   	return 0;
>   }
>
>

This patch is correct but it would be good to rework it to have common 
error exit point like in rte_pmd_init_internals() function you changed.

-- 
Pawel

[dpdk-dev] [PATCH v2] af_packet: Fix some klocwork errors

[Ouyang Changchun]

Fix possible memory leak issue: free kvlist before return;
Fix possible resource lost issue: close qssockfd before return;

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
change in v2:
  - Make the error exit point a common path.

 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf..c675724 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -694,6 +694,8 @@ error:
 		}
 		rte_free(*internals);
 	}
+	if (qsockfd != -1)
+		close(qsockfd);
 	return -1;
 }
 
@@ -823,16 +825,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
 		if (ret < 0)
-			return -1;
+			goto error;
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
 	if (ret < 0)
-		return -1;
+		goto error;
 
+	rte_kvargs_free(kvlist);
 	return 0;
+
+error:
+	rte_kvargs_free(kvlist);
+	return -1;
 }
 
 static struct rte_driver pmd_af_packet_drv = {
-- 
1.8.4.2

Re: [dpdk-dev] [PATCH v2] af_packet: Fix some klocwork errors

[Neil Horman]

On Fri, Feb 27, 2015 at 08:49:13AM +0800, Ouyang Changchun wrote:
> Fix possible memory leak issue: free kvlist before return;
> Fix possible resource lost issue: close qssockfd before return;
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
> change in v2:
>   - Make the error exit point a common path.
> 
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 80e9bdf..c675724 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -694,6 +694,8 @@ error:
>  		}
>  		rte_free(*internals);
>  	}
> +	if (qsockfd != -1)
> +		close(qsockfd);
This is insufficient.  qsockfd is a loop index that is reassigned for each tx
queue we have.  In the error path you need to do this, and loop through the tx
queues, closing each tx_queue->sockfd.

>  	return -1;
>  }
>  
> @@ -823,16 +825,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>  		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
>  		                         &open_packet_iface, &sockfd);
>  		if (ret < 0)
> -			return -1;
> +			goto error;
>  	}
>  
>  	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
>  	close(sockfd); /* no longer needed */
>  
>  	if (ret < 0)
> -		return -1;
> +		goto error;
>  
> +	rte_kvargs_free(kvlist);
>  	return 0;
> +
> +error:
> +	rte_kvargs_free(kvlist);
> +	return -1;
>  }
>  
>  static struct rte_driver pmd_af_packet_drv = {
> -- 
> 1.8.4.2
> 
> 

Re: [dpdk-dev] [PATCH v2] af_packet: Fix some klocwork errors

[Ouyang, Changchun]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Friday, February 27, 2015 10:05 PM
> To: Ouyang, Changchun
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] af_packet: Fix some klocwork errors
> 
> On Fri, Feb 27, 2015 at 08:49:13AM +0800, Ouyang Changchun wrote:
> > Fix possible memory leak issue: free kvlist before return; Fix
> > possible resource lost issue: close qssockfd before return;
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> > change in v2:
> >   - Make the error exit point a common path.
> >
> >  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > index 80e9bdf..c675724 100644
> > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > @@ -694,6 +694,8 @@ error:
> >  		}
> >  		rte_free(*internals);
> >  	}
> > +	if (qsockfd != -1)
> > +		close(qsockfd);
> This is insufficient.  qsockfd is a loop index that is reassigned for each tx
> queue we have.  In the error path you need to do this, and loop through the
> tx queues, closing each tx_queue->sockfd.
> 

Thanks, will have a v3 to resolve it.
Changchun

[dpdk-dev] [PATCH v3] af_packet: Fix some klocwork errors

[Ouyang Changchun]

Fix possible memory leak issue: free kvlist before return;
Fix possible resource lost issue: close qssockfd before return;

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
Change in v3:
  - Also close sockets for all queues.

Change in v2:
  - Make the error exit point a common path.

 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf..eb2b67b 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -691,9 +691,14 @@ error:
 				rte_free((*internals)->rx_queue[q].rd);
 			if ((*internals)->tx_queue[q].rd)
 				rte_free((*internals)->tx_queue[q].rd);
+			if (((*internals)->rx_queue[q].sockfd != -1) &&
+				((*internals)->rx_queue[q].sockfd != qsockfd))
+				close((*internals)->rx_queue[q].sockfd);
 		}
 		rte_free(*internals);
 	}
+	if (qsockfd != -1)
+		close(qsockfd);
 	return -1;
 }
 
@@ -823,16 +828,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
 		if (ret < 0)
-			return -1;
+			goto error;
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
 	if (ret < 0)
-		return -1;
+		goto error;
 
+	rte_kvargs_free(kvlist);
 	return 0;
+
+error:
+	rte_kvargs_free(kvlist);
+	return -1;
 }
 
 static struct rte_driver pmd_af_packet_drv = {
-- 
1.8.4.2

[dpdk-dev] [PATCH v4] af_packet: Fix some klocwork errors

[Ouyang Changchun]

Fix possible memory leak issue: free kvlist before return;
Fix possible resource lost issue: close qssockfd before return;

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
Change in v4:
  - Check sockfd in internals->rx_queue against 0.

Change in v3:
  - Also close sockets for all queues.
 
Change in v2:
  - Make the error exit point a common path.

 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf..acdf408 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -691,9 +691,14 @@ error:
 				rte_free((*internals)->rx_queue[q].rd);
 			if ((*internals)->tx_queue[q].rd)
 				rte_free((*internals)->tx_queue[q].rd);
+			if (((*internals)->rx_queue[q].sockfd != 0) &&
+				((*internals)->rx_queue[q].sockfd != qsockfd))
+				close((*internals)->rx_queue[q].sockfd);
 		}
 		rte_free(*internals);
 	}
+	if (qsockfd != -1)
+		close(qsockfd);
 	return -1;
 }
 
@@ -823,16 +828,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
 		if (ret < 0)
-			return -1;
+			goto error;
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
 	if (ret < 0)
-		return -1;
+		goto error;
 
+	rte_kvargs_free(kvlist);
 	return 0;
+
+error:
+	rte_kvargs_free(kvlist);
+	return -1;
 }
 
 static struct rte_driver pmd_af_packet_drv = {
-- 
1.8.4.2

Re: [dpdk-dev] [PATCH v4] af_packet: Fix some klocwork errors

[Qiu, Michael]

On 3/9/2015 11:49 AM, Ouyang Changchun wrote:
> Fix possible memory leak issue: free kvlist before return;
> Fix possible resource lost issue: close qssockfd before return;
>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
> Change in v4:
>   - Check sockfd in internals->rx_queue against 0.
>
> Change in v3:
>   - Also close sockets for all queues.
>  
> Change in v2:
>   - Make the error exit point a common path.
>
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 80e9bdf..acdf408 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -691,9 +691,14 @@ error:
>  				rte_free((*internals)->rx_queue[q].rd);
>  			if ((*internals)->tx_queue[q].rd)
>  				rte_free((*internals)->tx_queue[q].rd);
> +			if (((*internals)->rx_queue[q].sockfd != 0) &&
> +				((*internals)->rx_queue[q].sockfd != qsockfd))
> +				close((*internals)->rx_queue[q].sockfd);
>  		}
>  		rte_free(*internals);
>  	}
> +	if (qsockfd != -1)

Hi, Ouyang

qsockfd was first initialized:
for (q = 0; q < nb_queues; q++) {
                /* Open an AF_PACKET socket for this queue... */
                qsockfd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));

What will happen if gets an error before here, is there any issue? I
think better to initialize it when declare it.

Thanks,
Michael
> +		close(qsockfd);
>  	return -1;
>  }
>  
> @@ -823,16 +828,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>  		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
>  		                         &open_packet_iface, &sockfd);
>  		if (ret < 0)
> -			return -1;
> +			goto error;
>  	}
>  
>  	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
>  	close(sockfd); /* no longer needed */
>  
>  	if (ret < 0)
> -		return -1;
> +		goto error;
>  
> +	rte_kvargs_free(kvlist);
>  	return 0;
> +
> +error:
> +	rte_kvargs_free(kvlist);
> +	return -1;
>  }
>  
>  static struct rte_driver pmd_af_packet_drv = {

Re: [dpdk-dev] [PATCH v4] af_packet: Fix some klocwork errors

[Ouyang, Changchun]

Hi Michael,

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, March 9, 2015 2:39 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4] af_packet: Fix some klocwork errors
> 
> On 3/9/2015 11:49 AM, Ouyang Changchun wrote:
> > Fix possible memory leak issue: free kvlist before return; Fix
> > possible resource lost issue: close qssockfd before return;
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> > Change in v4:
> >   - Check sockfd in internals->rx_queue against 0.
> >
> > Change in v3:
> >   - Also close sockets for all queues.
> >
> > Change in v2:
> >   - Make the error exit point a common path.
> >
> >  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > index 80e9bdf..acdf408 100644
> > --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> > @@ -691,9 +691,14 @@ error:
> >  				rte_free((*internals)->rx_queue[q].rd);
> >  			if ((*internals)->tx_queue[q].rd)
> >  				rte_free((*internals)->tx_queue[q].rd);
> > +			if (((*internals)->rx_queue[q].sockfd != 0) &&
> > +				((*internals)->rx_queue[q].sockfd !=
> qsockfd))
> > +				close((*internals)->rx_queue[q].sockfd);
> >  		}
> >  		rte_free(*internals);
> >  	}
> > +	if (qsockfd != -1)
> 
> Hi, Ouyang
> 
> qsockfd was first initialized:
> for (q = 0; q < nb_queues; q++) {
>                 /* Open an AF_PACKET socket for this queue... */
>                 qsockfd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
> 
> What will happen if gets an error before here, is there any issue? I think
> better to initialize it when declare it.
>
Make sense,  will make next version to resolve it
Thanks
Changchun

 

[dpdk-dev] [PATCH v5] af_packet: Fix some klocwork errors

[Ouyang Changchun]

Fix possible memory leak issue: free kvlist before return;
Fix possible resource lost issue: close qssockfd before return;

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
Change in v5:
  - Initialize qsockfd with -1;

Change in v4:
  - Check sockfd in internals->rx_queue against 0.
 
Change in v3:
  - Also close sockets for all queues.
 
Change in v2:
  - Make the error exit point a common path.

 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf..2a27ebc 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -442,7 +442,8 @@ rte_pmd_init_internals(const char *name,
 	struct tpacket_req *req;
 	struct pkt_rx_queue *rx_queue;
 	struct pkt_tx_queue *tx_queue;
-	int rc, qsockfd, tpver, discard;
+	int rc, tpver, discard;
+	int qsockfd = -1;
 	unsigned int i, q, rdsize;
 	int fanout_arg __rte_unused, bypass __rte_unused;
 
@@ -691,9 +692,14 @@ error:
 				rte_free((*internals)->rx_queue[q].rd);
 			if ((*internals)->tx_queue[q].rd)
 				rte_free((*internals)->tx_queue[q].rd);
+			if (((*internals)->rx_queue[q].sockfd != 0) &&
+				((*internals)->rx_queue[q].sockfd != qsockfd))
+				close((*internals)->rx_queue[q].sockfd);
 		}
 		rte_free(*internals);
 	}
+	if (qsockfd != -1)
+		close(qsockfd);
 	return -1;
 }
 
@@ -823,16 +829,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
 		if (ret < 0)
-			return -1;
+			goto error;
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
 	if (ret < 0)
-		return -1;
+		goto error;
 
+	rte_kvargs_free(kvlist);
 	return 0;
+
+error:
+	rte_kvargs_free(kvlist);
+	return -1;
 }
 
 static struct rte_driver pmd_af_packet_drv = {
-- 
1.8.4.2

Re: [dpdk-dev] [PATCH v5] af_packet: Fix some klocwork errors

[John W. Linville]

On Mon, Mar 09, 2015 at 04:58:11PM +0800, Ouyang Changchun wrote:
> Fix possible memory leak issue: free kvlist before return;
> Fix possible resource lost issue: close qssockfd before return;
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
> Change in v5:
>   - Initialize qsockfd with -1;
> 
> Change in v4:
>   - Check sockfd in internals->rx_queue against 0.
>  
> Change in v3:
>   - Also close sockets for all queues.
>  
> Change in v2:
>   - Make the error exit point a common path.
> 
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 80e9bdf..2a27ebc 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -442,7 +442,8 @@ rte_pmd_init_internals(const char *name,
>  	struct tpacket_req *req;
>  	struct pkt_rx_queue *rx_queue;
>  	struct pkt_tx_queue *tx_queue;
> -	int rc, qsockfd, tpver, discard;
> +	int rc, tpver, discard;
> +	int qsockfd = -1;
>  	unsigned int i, q, rdsize;
>  	int fanout_arg __rte_unused, bypass __rte_unused;
>  
> @@ -691,9 +692,14 @@ error:
>  				rte_free((*internals)->rx_queue[q].rd);
>  			if ((*internals)->tx_queue[q].rd)
>  				rte_free((*internals)->tx_queue[q].rd);
> +			if (((*internals)->rx_queue[q].sockfd != 0) &&
> +				((*internals)->rx_queue[q].sockfd != qsockfd))
> +				close((*internals)->rx_queue[q].sockfd);
>  		}
>  		rte_free(*internals);
>  	}
> +	if (qsockfd != -1)
> +		close(qsockfd);
>  	return -1;
>  }
>  

The part above seems fine.

> @@ -823,16 +829,21 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>  		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
>  		                         &open_packet_iface, &sockfd);
>  		if (ret < 0)
> -			return -1;
> +			goto error;
>  	}
>  
>  	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
>  	close(sockfd); /* no longer needed */
>  
>  	if (ret < 0)
> -		return -1;
> +		goto error;
>  
> +	rte_kvargs_free(kvlist);
>  	return 0;
> +
> +error:
> +	rte_kvargs_free(kvlist);
> +	return -1;
>  }
>  
>  static struct rte_driver pmd_af_packet_drv = {

This seems a bit awkward.  How about the following?

@@ -823,16 +829,15 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
 		if (ret < 0)
-			return -1;
+			goto exit;
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
-	if (ret < 0)
-		return -1;
-
-	return 0;
+exit:
+	rte_kvargs_free(kvlist);
+	return ret;
 }
 
 static struct rte_driver pmd_af_packet_drv = {

Does this seem OK to you?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors

[Ouyang Changchun]

Fix possible memory leak issue: free kvlist before return;
Fix possible resource lost issue: close qssockfd before return;

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
Change in v6:
  - Refine exit point;

Change in v5:
  - Initialize qsockfd with -1;
 
Change in v4:
  - Check sockfd in internals->rx_queue against 0.
 
Change in v3:
  - Also close sockets for all queues.
 
Change in v2:
  - Make the error exit point a common path.

 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf..c2c0878 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -442,7 +442,8 @@ rte_pmd_init_internals(const char *name,
 	struct tpacket_req *req;
 	struct pkt_rx_queue *rx_queue;
 	struct pkt_tx_queue *tx_queue;
-	int rc, qsockfd, tpver, discard;
+	int rc, tpver, discard;
+	int qsockfd = -1;
 	unsigned int i, q, rdsize;
 	int fanout_arg __rte_unused, bypass __rte_unused;
 
@@ -691,9 +692,14 @@ error:
 				rte_free((*internals)->rx_queue[q].rd);
 			if ((*internals)->tx_queue[q].rd)
 				rte_free((*internals)->tx_queue[q].rd);
+			if (((*internals)->rx_queue[q].sockfd != 0) &&
+				((*internals)->rx_queue[q].sockfd != qsockfd))
+				close((*internals)->rx_queue[q].sockfd);
 		}
 		rte_free(*internals);
 	}
+	if (qsockfd != -1)
+		close(qsockfd);
 	return -1;
 }
 
@@ -802,7 +808,7 @@ int
 rte_pmd_af_packet_devinit(const char *name, const char *params)
 {
 	unsigned numa_node;
-	int ret;
+	int ret = 0;
 	struct rte_kvargs *kvlist;
 	int sockfd = -1;
 
@@ -811,8 +817,10 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 	numa_node = rte_socket_id();
 
 	kvlist = rte_kvargs_parse(params, valid_arguments);
-	if (kvlist == NULL)
-		return -1;
+	if (kvlist == NULL) {
+		ret = -1;
+		goto exit;
+	}
 
 	/*
 	 * If iface argument is passed we open the NICs and use them for
@@ -823,16 +831,16 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
 		if (ret < 0)
-			return -1;
+			goto exit;
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
-	if (ret < 0)
-		return -1;
-
-	return 0;
+exit:
+	if (kvlist != NULL)
+		rte_kvargs_free(kvlist);
+	return ret;
 }
 
 static struct rte_driver pmd_af_packet_drv = {
-- 
1.8.4.2

Re: [dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors

[Pawel Wodkowski]

> -
> -	return 0;
> +exit:
> +	if (kvlist != NULL)

No need for if(). This part was fine previous patch.

> +		rte_kvargs_free(kvlist);
> +	return ret;
>   }
>
>   static struct rte_driver pmd_af_packet_drv = {
>


-- 
Pawel

Re: [dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors

[Ouyang, Changchun]

> -----Original Message-----
> From: Wodkowski, PawelX
> Sent: Tuesday, March 10, 2015 4:37 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Cc: linville@tuxdriver.com; nhorman@tuxdriver.com
> Subject: Re: [PATCH v6] af_packet: Fix some klocwork errors
> 
> > -
> > -	return 0;
> > +exit:
> > +	if (kvlist != NULL)
> 
> No need for if(). This part was fine previous patch.
>

If kvlist is NULL, no reason to call rte_kvargs_free to free it.
So, adding this test is better.
 
> > +		rte_kvargs_free(kvlist);
> > +	return ret;
> >   }
> >
> >   static struct rte_driver pmd_af_packet_drv = {
> >
> 
> 
> --
> Pawel

Re: [dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors

[Pawel Wodkowski]

On 2015-03-10 09:49, Ouyang, Changchun wrote:
>
>
>> -----Original Message-----
>> From: Wodkowski, PawelX
>> Sent: Tuesday, March 10, 2015 4:37 PM
>> To: Ouyang, Changchun; dev@dpdk.org
>> Cc: linville@tuxdriver.com; nhorman@tuxdriver.com
>> Subject: Re: [PATCH v6] af_packet: Fix some klocwork errors
>>
>>> -
>>> -	return 0;
>>> +exit:
>>> +	if (kvlist != NULL)
>>
>> No need for if(). This part was fine previous patch.
>>
>
> If kvlist is NULL, no reason to call rte_kvargs_free to free it.
> So, adding this test is better.

For programmer convenience and reduce code bloat/obfuscation the same 
test is in rte_kvargs_free() (and every other free-like function). If 
there is no particular reason for that (ex performance which is not in 
this path) checking pointer for NULL value should be avoided before 
freeing it.

>
>>> +		rte_kvargs_free(kvlist);
>>> +	return ret;
>>>    }
>>>
>>>    static struct rte_driver pmd_af_packet_drv = {
>>>
>>
>>
>> --
>> Pawel


-- 
Pawel

Re: [dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors

[Thomas Monjalon]

2015-03-10 08:49, Ouyang, Changchun:
> From: Wodkowski, PawelX
> > > +	if (kvlist != NULL)
> > 
> > No need for if(). This part was fine previous patch.
> >
> 
> If kvlist is NULL, no reason to call rte_kvargs_free to free it.
> So, adding this test is better.

No, we don't need to double check.
1/ it's already checked in rte_kvargs_free()
2/ less lines you write, better it is

> > > +		rte_kvargs_free(kvlist);
> > > +	return ret;
> > >   }

Re: [dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors

[Ouyang, Changchun]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, March 10, 2015 6:20 PM
> To: Ouyang, Changchun
> Cc: dev@dpdk.org; Wodkowski, PawelX
> Subject: Re: [dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors
> 
> 2015-03-10 08:49, Ouyang, Changchun:
> > From: Wodkowski, PawelX
> > > > +	if (kvlist != NULL)
> > >
> > > No need for if(). This part was fine previous patch.
> > >
> >
> > If kvlist is NULL, no reason to call rte_kvargs_free to free it.
> > So, adding this test is better.
> 
> No, we don't need to double check.
> 1/ it's already checked in rte_kvargs_free() 2/ less lines you write, better it is
> 

At least 2 guys vote for removing the check, then 2 vs. 1, you win :-)
Will update it in v7

Thanks for comments!
Changchun
 

[dpdk-dev] [PATCH v7] af_packet: Fix some klocwork errors

[Ouyang Changchun]

Fix possible memory leak issue: free kvlist before return;
Fix possible resource lost issue: close qssockfd before return;

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
Change in v7:
  - Remove unnecessary '!= NULL' check before freeing it; 

Change in v6:
  - Refine exit point;
 
Change in v5:
  - Initialize qsockfd with -1;
 
Change in v4:
  - Check sockfd in internals->rx_queue against 0;
 
Change in v3:
  - Also close sockets for all queues;
 
Change in v2:
  - Make the error exit point a common path.

 lib/librte_pmd_af_packet/rte_eth_af_packet.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
index 80e9bdf..2ac50ba 100644
--- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
+++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
@@ -442,7 +442,8 @@ rte_pmd_init_internals(const char *name,
 	struct tpacket_req *req;
 	struct pkt_rx_queue *rx_queue;
 	struct pkt_tx_queue *tx_queue;
-	int rc, qsockfd, tpver, discard;
+	int rc, tpver, discard;
+	int qsockfd = -1;
 	unsigned int i, q, rdsize;
 	int fanout_arg __rte_unused, bypass __rte_unused;
 
@@ -691,9 +692,14 @@ error:
 				rte_free((*internals)->rx_queue[q].rd);
 			if ((*internals)->tx_queue[q].rd)
 				rte_free((*internals)->tx_queue[q].rd);
+			if (((*internals)->rx_queue[q].sockfd != 0) &&
+				((*internals)->rx_queue[q].sockfd != qsockfd))
+				close((*internals)->rx_queue[q].sockfd);
 		}
 		rte_free(*internals);
 	}
+	if (qsockfd != -1)
+		close(qsockfd);
 	return -1;
 }
 
@@ -802,7 +808,7 @@ int
 rte_pmd_af_packet_devinit(const char *name, const char *params)
 {
 	unsigned numa_node;
-	int ret;
+	int ret = 0;
 	struct rte_kvargs *kvlist;
 	int sockfd = -1;
 
@@ -811,8 +817,10 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 	numa_node = rte_socket_id();
 
 	kvlist = rte_kvargs_parse(params, valid_arguments);
-	if (kvlist == NULL)
-		return -1;
+	if (kvlist == NULL) {
+		ret = -1;
+		goto exit;
+	}
 
 	/*
 	 * If iface argument is passed we open the NICs and use them for
@@ -823,16 +831,15 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
 		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
 		                         &open_packet_iface, &sockfd);
 		if (ret < 0)
-			return -1;
+			goto exit;
 	}
 
 	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
 	close(sockfd); /* no longer needed */
 
-	if (ret < 0)
-		return -1;
-
-	return 0;
+exit:
+	rte_kvargs_free(kvlist);
+	return ret;
 }
 
 static struct rte_driver pmd_af_packet_drv = {
-- 
1.8.4.2

Re: [dpdk-dev] [PATCH v7] af_packet: Fix some klocwork errors

[Qiu, Michael]

On 3/11/2015 9:34 AM, Ouyang Changchun wrote:
> Fix possible memory leak issue: free kvlist before return;
> Fix possible resource lost issue: close qssockfd before return;
>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
> Change in v7:
>   - Remove unnecessary '!= NULL' check before freeing it; 
>
> Change in v6:
>   - Refine exit point;
>  
> Change in v5:
>   - Initialize qsockfd with -1;
>  
> Change in v4:
>   - Check sockfd in internals->rx_queue against 0;
>  
> Change in v3:
>   - Also close sockets for all queues;
>  
> Change in v2:
>   - Make the error exit point a common path.

Acked-by: Michael Qiu <michael.qiu@intel.com>
>  lib/librte_pmd_af_packet/rte_eth_af_packet.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> index 80e9bdf..2ac50ba 100644
> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c
> @@ -442,7 +442,8 @@ rte_pmd_init_internals(const char *name,
>  	struct tpacket_req *req;
>  	struct pkt_rx_queue *rx_queue;
>  	struct pkt_tx_queue *tx_queue;
> -	int rc, qsockfd, tpver, discard;
> +	int rc, tpver, discard;
> +	int qsockfd = -1;
>  	unsigned int i, q, rdsize;
>  	int fanout_arg __rte_unused, bypass __rte_unused;
>  
> @@ -691,9 +692,14 @@ error:
>  				rte_free((*internals)->rx_queue[q].rd);
>  			if ((*internals)->tx_queue[q].rd)
>  				rte_free((*internals)->tx_queue[q].rd);
> +			if (((*internals)->rx_queue[q].sockfd != 0) &&
> +				((*internals)->rx_queue[q].sockfd != qsockfd))
> +				close((*internals)->rx_queue[q].sockfd);
>  		}
>  		rte_free(*internals);
>  	}
> +	if (qsockfd != -1)
> +		close(qsockfd);
>  	return -1;
>  }
>  
> @@ -802,7 +808,7 @@ int
>  rte_pmd_af_packet_devinit(const char *name, const char *params)
>  {
>  	unsigned numa_node;
> -	int ret;
> +	int ret = 0;
>  	struct rte_kvargs *kvlist;
>  	int sockfd = -1;
>  
> @@ -811,8 +817,10 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>  	numa_node = rte_socket_id();
>  
>  	kvlist = rte_kvargs_parse(params, valid_arguments);
> -	if (kvlist == NULL)
> -		return -1;
> +	if (kvlist == NULL) {
> +		ret = -1;
> +		goto exit;
> +	}
>  
>  	/*
>  	 * If iface argument is passed we open the NICs and use them for
> @@ -823,16 +831,15 @@ rte_pmd_af_packet_devinit(const char *name, const char *params)
>  		ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
>  		                         &open_packet_iface, &sockfd);
>  		if (ret < 0)
> -			return -1;
> +			goto exit;
>  	}
>  
>  	ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
>  	close(sockfd); /* no longer needed */
>  
> -	if (ret < 0)
> -		return -1;
> -
> -	return 0;
> +exit:
> +	rte_kvargs_free(kvlist);
> +	return ret;
>  }
>  
>  static struct rte_driver pmd_af_packet_drv = {

Re: [dpdk-dev] [PATCH v7] af_packet: Fix some klocwork errors

[Neil Horman]

On Thu, Mar 12, 2015 at 10:43:24AM +0000, Qiu, Michael wrote:
> On 3/11/2015 9:34 AM, Ouyang Changchun wrote:
> > Fix possible memory leak issue: free kvlist before return;
> > Fix possible resource lost issue: close qssockfd before return;
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> > Change in v7:
> >   - Remove unnecessary '!= NULL' check before freeing it; 
> >
> > Change in v6:
> >   - Refine exit point;
> >  
> > Change in v5:
> >   - Initialize qsockfd with -1;
> >  
> > Change in v4:
> >   - Check sockfd in internals->rx_queue against 0;
> >  
> > Change in v3:
> >   - Also close sockets for all queues;
> >  
> > Change in v2:
> >   - Make the error exit point a common path.
> 
> Acked-by: Michael Qiu <michael.qiu@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [dpdk-dev] [PATCH v7] af_packet: Fix some klocwork errors

[John W. Linville]

On Thu, Mar 12, 2015 at 09:38:46AM -0400, Neil Horman wrote:
> On Thu, Mar 12, 2015 at 10:43:24AM +0000, Qiu, Michael wrote:
> > On 3/11/2015 9:34 AM, Ouyang Changchun wrote:
> > > Fix possible memory leak issue: free kvlist before return;
> > > Fix possible resource lost issue: close qssockfd before return;
> > >
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > ---
> > > Change in v7:
> > >   - Remove unnecessary '!= NULL' check before freeing it; 
> > >
> > > Change in v6:
> > >   - Refine exit point;
> > >  
> > > Change in v5:
> > >   - Initialize qsockfd with -1;
> > >  
> > > Change in v4:
> > >   - Check sockfd in internals->rx_queue against 0;
> > >  
> > > Change in v3:
> > >   - Also close sockets for all queues;
> > >  
> > > Change in v2:
> > >   - Make the error exit point a common path.
> > 
> > Acked-by: Michael Qiu <michael.qiu@intel.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Acked-by: John W. Linville <linville@tuxdriver.com>

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [dpdk-dev] [PATCH v7] af_packet: Fix some klocwork errors

[Thomas Monjalon]

> > > > Fix possible memory leak issue: free kvlist before return;
> > > > Fix possible resource lost issue: close qssockfd before return;
> > > >
> > > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > > ---
> > > > Change in v7:
> > > >   - Remove unnecessary '!= NULL' check before freeing it; 
> > > >
> > > > Change in v6:
> > > >   - Refine exit point;
> > > >  
> > > > Change in v5:
> > > >   - Initialize qsockfd with -1;
> > > >  
> > > > Change in v4:
> > > >   - Check sockfd in internals->rx_queue against 0;
> > > >  
> > > > Change in v3:
> > > >   - Also close sockets for all queues;
> > > >  
> > > > Change in v2:
> > > >   - Make the error exit point a common path.
> > > 
> > > Acked-by: Michael Qiu <michael.qiu@intel.com>
> > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> 
> Acked-by: John W. Linville <linville@tuxdriver.com>

Applied, thanks