Re: [PATCH] pcap, af_packet, null: Fix memory leaks of 'struct rte_kvargs'

[Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>
> 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-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
>
>
>> ---
>>  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
>>
>>