From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tetsuya Mukawa Subject: Re: [PATCH] pcap, af_packet, null: Fix memory leaks of 'struct rte_kvargs' Date: Tue, 17 Mar 2015 12:53:42 +0900 Message-ID: <5507A546.4050307@igel.co.jp> References: <1426477376-13385-1-git-send-email-mukawa@igel.co.jp> <20150316104710.GA16238@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: dev-VfR2kkLFssw@public.gmane.org To: Neil Horman Return-path: In-Reply-To: <20150316104710.GA16238-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On 2015/03/16 19:47, Neil Horman wrote: > On Mon, Mar 16, 2015 at 12:42:56PM +0900, Tetsuya Mukawa wrote: >> 'struct rte_kvargs' is allocated in rte_kvargs_parse(), and should be >> freed with rte_kvargs_free(). >> This patch fixes memory leaks of 'struct rte_kvargs' in below PMDs. >> - pcap PMD >> - af_packet PMD >> - null PMD >> >> Reported-by: Mcnamara, John >> Signed-off-by: Tetsuya Mukawa > Typically, you should limit your goto tags to something more minimalist= that > describes the one thing you want to unwind at the label. As it stands = here, if > somone wants to add some code after this label, the label either become= s > incorrect, or you need to fix up the label name to match. But I suppos= e that > can be corrected later. Hi Neil, I appreciate for your suggestion. I will need to remove af_packet fixing and submit the patch again. So I will follow your suggestion in the next patch. Thanks, Tetsuya > Acked-by: Neil Horman > > >> --- >> lib/librte_pmd_af_packet/rte_eth_af_packet.c | 5 ++++- >> lib/librte_pmd_null/rte_eth_null.c | 13 +++++++++---- >> lib/librte_pmd_pcap/rte_eth_pcap.c | 18 +++++++++++------- >> 3 files changed, 24 insertions(+), 12 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..7c3c6a9 100644 >> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c >> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c >> @@ -823,12 +823,15 @@ rte_pmd_af_packet_devinit(const char *name, cons= t char *params) >> ret =3D rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG, >> &open_packet_iface, &sockfd); >> if (ret < 0) >> - return -1; >> + goto free_kvlist_then_return; >> } >> =20 >> ret =3D rte_eth_from_packet(name, &sockfd, numa_node, kvlist); >> close(sockfd); /* no longer needed */ >> =20 >> +free_kvlist_then_return: >> + rte_kvargs_free(kvlist); >> + >> if (ret < 0) >> return -1; >> =20 >> diff --git a/lib/librte_pmd_null/rte_eth_null.c b/lib/librte_pmd_null/= rte_eth_null.c >> index f49d686..86307eb 100644 >> --- a/lib/librte_pmd_null/rte_eth_null.c >> +++ b/lib/librte_pmd_null/rte_eth_null.c >> @@ -506,7 +506,7 @@ rte_pmd_null_devinit(const char *name, const char = *params) >> unsigned numa_node; >> unsigned packet_size =3D default_packet_size; >> unsigned packet_copy =3D default_packet_copy; >> - struct rte_kvargs *kvlist; >> + struct rte_kvargs *kvlist =3D NULL; >> int ret; >> =20 >> if (name =3D=3D NULL) >> @@ -527,7 +527,7 @@ rte_pmd_null_devinit(const char *name, const char = *params) >> ETH_NULL_PACKET_SIZE_ARG, >> &get_packet_size_arg, &packet_size); >> if (ret < 0) >> - return -1; >> + goto free_kvlist_then_return; >> } >> =20 >> if (rte_kvargs_count(kvlist, ETH_NULL_PACKET_COPY_ARG) =3D=3D 1) { >> @@ -536,7 +536,7 @@ rte_pmd_null_devinit(const char *name, const char = *params) >> ETH_NULL_PACKET_COPY_ARG, >> &get_packet_copy_arg, &packet_copy); >> if (ret < 0) >> - return -1; >> + goto free_kvlist_then_return; >> } >> } >> =20 >> @@ -544,7 +544,12 @@ rte_pmd_null_devinit(const char *name, const char= *params) >> "packet copy is %s\n", packet_size, >> packet_copy ? "enabled" : "disabled"); >> =20 >> - return eth_dev_null_create(name, numa_node, packet_size, packet_copy= ); >> + ret =3D eth_dev_null_create(name, numa_node, packet_size, packet_cop= y); >> + >> +free_kvlist_then_return: >> + if (kvlist) >> + rte_kvargs_free(kvlist); >> + return ret; >> } >> =20 >> static int >> diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/= rte_eth_pcap.c >> index 5e94930..de15fb3 100644 >> --- a/lib/librte_pmd_pcap/rte_eth_pcap.c >> +++ b/lib/librte_pmd_pcap/rte_eth_pcap.c >> @@ -888,12 +888,13 @@ rte_pmd_pcap_devinit(const char *name, const cha= r *params) >> ret =3D rte_kvargs_process(kvlist, ETH_PCAP_IFACE_ARG, >> &open_rx_tx_iface, &pcaps); >> if (ret < 0) >> - return -1; >> + goto free_kvlist_then_return; >> dumpers.pcaps[0] =3D pcaps.pcaps[0]; >> dumpers.names[0] =3D pcaps.names[0]; >> dumpers.types[0] =3D pcaps.types[0]; >> - return rte_eth_from_pcaps(name, &pcaps, 1, &dumpers, 1, >> + ret =3D rte_eth_from_pcaps(name, &pcaps, 1, &dumpers, 1, >> numa_node, kvlist, 1); >> + goto free_kvlist_then_return; >> } >> =20 >> /* >> @@ -911,7 +912,7 @@ rte_pmd_pcap_devinit(const char *name, const char = *params) >> } >> =20 >> if (ret < 0) >> - return -1; >> + goto free_kvlist_then_return; >> =20 >> /* >> * We check whether we want to open a TX stream to a real NIC or a >> @@ -930,15 +931,18 @@ rte_pmd_pcap_devinit(const char *name, const cha= r *params) >> } >> =20 >> if (ret < 0) >> - return -1; >> + goto free_kvlist_then_return; >> =20 >> if (using_dumpers) >> - return rte_eth_from_pcaps_n_dumpers(name, &pcaps, pcaps.num_of_rx, >> + ret =3D rte_eth_from_pcaps_n_dumpers(name, &pcaps, pcaps.num_of_rx,= >> &dumpers, dumpers.num_of_tx, numa_node, kvlist); >> - >> - return rte_eth_from_pcaps(name, &pcaps, pcaps.num_of_rx, &dumpers, >> + else >> + ret =3D rte_eth_from_pcaps(name, &pcaps, pcaps.num_of_rx, &dumpers,= >> dumpers.num_of_tx, numa_node, kvlist, 0); >> =20 >> +free_kvlist_then_return: >> + rte_kvargs_free(kvlist); >> + return ret; >> } >> =20 >> static int >> --=20 >> 1.9.1 >> >>