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
