[dpdk-dev] [PATCH v4] af_packet: Fix some klocwork errors
Fix possible memory leak issue: free kvlist before return; Fix possible resource lost issue: close qssockfd before return; Signed-off-by: Changchun Ouyang --- 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
[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 > --- > 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 = {
[dpdk-dev] [PATCH v4] af_packet: Fix some klocwork errors
Hi Michael, > -Original Message- > From: Qiu, Michael > Sent: Monday, March 9, 2015 2:39 PM > To: Ouyang, Changchun; dev at 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 > > --- > > 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