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 <john.mcnamara at intel.com>
>> Signed-off-by: Tetsuya Mukawa <mukawa at igel.co.jp>
> 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 becomes
> incorrect, or you need to fix up the label name to match. But I suppose 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 <nhorman at tuxdriver.com>
>
>
>> ---
>> 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, const char
>> *params)
>> ret = rte_kvargs_process(kvlist, ETH_AF_PACKET_IFACE_ARG,
>> &open_packet_iface, &sockfd);
>> if (ret < 0)
>> - return -1;
>> + goto free_kvlist_then_return;
>> }
>>
>> ret = rte_eth_from_packet(name, &sockfd, numa_node, kvlist);
>> close(sockfd); /* no longer needed */
>>
>> +free_kvlist_then_return:
>> + rte_kvargs_free(kvlist);
>> +
>> if (ret < 0)
>> return -1;
>>
>> 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 = default_packet_size;
>> unsigned packet_copy = default_packet_copy;
>> - struct rte_kvargs *kvlist;
>> + struct rte_kvargs *kvlist = NULL;
>> int ret;
>>
>> if (name == 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;
>> }
>>
>> if (rte_kvargs_count(kvlist, ETH_NULL_PACKET_COPY_ARG) == 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;
>> }
>> }
>>
>> @@ -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");
>>
>> - return eth_dev_null_create(name, numa_node, packet_size, packet_copy);
>> + ret = eth_dev_null_create(name, numa_node, packet_size, packet_copy);
>> +
>> +free_kvlist_then_return:
>> + if (kvlist)
>> + rte_kvargs_free(kvlist);
>> + return ret;
>> }
>>
>> 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 char
>> *params)
>> ret = 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] = pcaps.pcaps[0];
>> dumpers.names[0] = pcaps.names[0];
>> dumpers.types[0] = pcaps.types[0];
>> - return rte_eth_from_pcaps(name, &pcaps, 1, &dumpers, 1,
>> + ret = rte_eth_from_pcaps(name, &pcaps, 1, &dumpers, 1,
>> numa_node, kvlist, 1);
>> + goto free_kvlist_then_return;
>> }
>>
>> /*
>> @@ -911,7 +912,7 @@ rte_pmd_pcap_devinit(const char *name, const char
>> *params)
>> }
>>
>> if (ret < 0)
>> - return -1;
>> + goto free_kvlist_then_return;
>>
>> /*
>> * 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 char
>> *params)
>> }
>>
>> if (ret < 0)
>> - return -1;
>> + goto free_kvlist_then_return;
>>
>> if (using_dumpers)
>> - return rte_eth_from_pcaps_n_dumpers(name, &pcaps,
>> pcaps.num_of_rx,
>> + ret = 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 = rte_eth_from_pcaps(name, &pcaps, pcaps.num_of_rx,
>> &dumpers,
>> dumpers.num_of_tx, numa_node, kvlist, 0);
>>
>> +free_kvlist_then_return:
>> + rte_kvargs_free(kvlist);
>> + return ret;
>> }
>>
>> static int
>> --
>> 1.9.1
>>
>>
