Re: [PATCH 3/6] eal: Fix memory leaks and needless incrementation of pci uio implementation

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

On 2015/03/19 0:39, David Marchand wrote:
> On Tue, Mar 17, 2015 at 10:30 AM, Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org
> <mailto:mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>> wrote:
>
>     When pci_map_resource() is failed but path is allocated correctly,
>     path won't be freed. Also, when open() is failed, uio_res won't be
>     freed.
>     This patch fixes these memory leaks.
>     When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
>     In this case, pci_map_addr should not be incremented.
>
>     Also, the patch fixes belows.
>      - To shrink code, move close().
>      - Remove fail variable.
>
>     Signed-off-by: Tetsuya Mukawa <mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org
>     <mailto:mukawa-AlSX/UN32fvPDbFq/vQRIQ@public.gmane.org>>
>     ---
>      lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 28
>     ++++++++++++++--------------
>      1 file changed, 14 insertions(+), 14 deletions(-)
>
>     diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     index b971ec9..5044884 100644
>     --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>     @@ -333,7 +333,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>             maps = uio_res->maps;
>             for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
>                     int fd;
>     -               int fail = 0;
>
>                     /* skip empty BAR */
>                     phaddr = dev->mem_resource[i].phys_addr;
>     @@ -347,6 +346,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>                                     loc->domain, loc->bus, loc->devid,
>     loc->function,
>                                     i);
>
>     +               /* allocate memory to keep path */
>     +               maps[map_idx].path = rte_malloc(NULL,
>     strlen(devname) + 1, 0);
>     +               if (maps[map_idx].path == NULL)
>     +                       goto fail0;
>     +
>
>
>  [snip]
>
> Neither fail0 nor fail1 labels seem to free previous allocations.
> Did I miss something ?

Hi David,


Ah, you are right. I will free all 'maps[i].path' allocated in this
function.

Thanks,
Tetsuya


>
> [snip]
>
>     +
>     +fail1:
>     +       rte_free(maps[map_idx].path);
>     +fail0:
>     +       rte_free(uio_res);
>     +       return -1;
>      }
>
>
>  
>
> -- 
> David Marchand