Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

["Kerlin, MarcinX" <marcinx.kerlin@intel.com>]

> -----Original Message-----
> From: Gonzalez Monroy, Sergio
> Sent: Thursday, April 21, 2016 1:19 PM
> To: David Marchand <david.marchand@6wind.com>
> Cc: dev@dpdk.org; Kerlin, MarcinX <marcinx.kerlin@intel.com>
> Subject: Re: [PATCH 1/1] lib/librte_eal: fix resource leak
> 
> On 20/04/2016 10:15, David Marchand wrote:
> > On Tue, Apr 19, 2016 at 6:27 PM, Marcin Kerlin <marcinx.kerlin@intel.com>
> wrote:
> >> Fix issue reported by Coverity.
> >>
> >> Coverity ID 13295, 13296, 13303:
> >> Resource leak: The system resource will not be reclaimed and reused,
> >> reducing the future availability of the resource.
> >> In rte_eal_hugepage_attach: Leak of memory or pointers to system
> >> resources.
> >>
> >> Fixes: af75078fece3 ("first public release")
> >>
> >> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
> >> ---
> >>   lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +++++++++++-
> >>   1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> b/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> index 5b9132c..6320aa0 100644
> >> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> >> @@ -1475,13 +1475,17 @@ rte_eal_hugepage_attach(void)
> >>                                          "and retry running both primary "
> >>                                          "and secondary processes\n");
> >>                          }
> >> +
> >> +                       if (base_addr != MAP_FAILED)
> >> +                               munmap((void *)(uintptr_t)base_addr,
> >> + mcfg->memseg[s].len);
> >> +
> > What is the point of this casting ?
> > Idem for the rest of the patch.
> 
> I don't see the point either.
> Marcin?

Oh sorry, right, an oversight with the redundant casting.

> 
> >
> > I can't see cleanup for previously mapped segments when mapping one fails.
> > Do we want this cleanup as well ?
> 
> Good point.
> 
> We are not really leaking resources because we panic if we fail to initialize eal
> memory.
> 
> Now, if we are going to do the cleanup, I think that as David points out we
> should be cleaning up all previous mappings too.

Exactly app panic after fail so do we need to worry about these warnings from Coverity and try to improve or leave it without affecting?

> 
> Sergio
> > CC Sergio.
> >
> >