Re: [dpdk-dev] [PATCH v7 03/12] eal: Fix memory leaks and needless increment of pci_map_addr

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/07/02 18:57, Bruce Richardson wrote:
> On Tue, Jun 30, 2015 at 05:24:19PM +0900, Tetsuya Mukawa wrote:
>> ---
>>  lib/librte_eal/bsdapp/eal/eal_pci.c       | 14 ++++++--
>>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 56 ++++++++++++++++++++-----------
>>  2 files changed, 48 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
>> index 8e24fd1..b071f07 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
>> @@ -235,7 +235,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  	if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) {
>>  		RTE_LOG(ERR, EAL,
>>  			"%s(): cannot store uio mmap details\n", __func__);
>> -		return -1;
>> +		goto close_fd;
>>  	}
>>  
>>  	snprintf(uio_res->path, sizeof(uio_res->path), "%s", devname);
>> @@ -262,8 +262,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  		    (mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
>>  						(size_t)maps[j].size)
>>  		    ) == NULL) {
>> -			rte_free(uio_res);
>> -			return -1;
>> +			goto free_uio_res;
>>  		}
>>  
>>  		maps[j].addr = mapaddr;
>> @@ -274,6 +273,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  	TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
>>  
>>  	return 0;
>> +
>> +free_uio_res:
>> +	rte_free(uio_res);
> If you initialize uio_res to NULL when it is defined, you can get rid of the
> distinction between "free_uio_res" and "close_fd" labels. 
>
> Similarly, if you check for a valid dev->intr_handle.fd value before close, you can have a 
> generic "error" leg, and replace all return -1's with goto error. While not as
> useful as merging the labels, it might be something to consider.

Hi Bruce,

Thanks for comments.
Sure, I will cleanup my patch like your mentioned. It will be easier to
read.

>> @@ -368,23 +373,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  
>>  		mapaddr = pci_map_resource(pci_map_addr, fd, 0,
>>  				(size_t)dev->mem_resource[i].len, 0);
>> -		if (mapaddr == MAP_FAILED)
>> -			fail = 1;
>> +		close(fd);
>> +		if (mapaddr == MAP_FAILED) {
>> +			rte_free(maps[map_idx].path);
>> +			goto free_uio_res;
>> +		}
>>  
>>  		pci_map_addr = RTE_PTR_ADD(mapaddr,
>>  				(size_t)dev->mem_resource[i].len);
>>  
>> -		maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
>> -		if (maps[map_idx].path == NULL)
>> -			fail = 1;
>> -
>> -		if (fail) {
>> -			rte_free(uio_res);
>> -			close(fd);
>> -			return -1;
>> -		}
>> -		close(fd);
>> -
>>  		maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
>>  		maps[map_idx].size = dev->mem_resource[i].len;
>>  		maps[map_idx].addr = mapaddr;
>> @@ -399,6 +396,25 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>  	TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
>>  
>>  	return 0;
>> +
>> +free_uio_res:
>> +	for (i = 0; i < map_idx; i++) {
> If you move the initialization of map_idx = 0 from the "for" loop to the definition
> at the start of the function, you may again be able to merge these two labels
> into one.

Also I will fix it in next patches.

Regards,
Tetsuya