[dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, March 10, 2015 6:20 PM > To: Ouyang, Changchun > Cc: dev at 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 v6] 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 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, _packet_iface, ); if (ret < 0) - return -1; + goto exit; } ret = rte_eth_from_packet(name, , 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
[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 > > > + rte_kvargs_free(kvlist); > > > + return ret; > > > }
[dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors
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 at dpdk.org >> Cc: linville at tuxdriver.com; nhorman at 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
[dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors
> - > - 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
[dpdk-dev] [PATCH v6] af_packet: Fix some klocwork errors
> -Original Message- > From: Wodkowski, PawelX > Sent: Tuesday, March 10, 2015 4:37 PM > To: Ouyang, Changchun; dev at dpdk.org > Cc: linville at tuxdriver.com; nhorman at 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