Re: [Qemu-devel] [PATCH] hw/arm/realview.c: Fix memory leak in realview_init()

[Kirill Batuzov]

> On 20 November 2014 11:53, Kirill Batuzov <address@hidden> wrote:
> > I'm surprised that this small patch caused so much controversy. It seems
> > very simple and straightforward to me.
> >
> > This patch fixes a memory leak. The fact that it indeed was a memory
> > leak is indicated by Valgrind output (Memcheck's false-positives are
> > extremely rare unless you do some really nasty things with your pointers).
> > It can be verified manually too: there are only 4 occurrences of 'ram_lo'
> > in realview.c.
> 
> It's in exactly the same situation as the other blocks of memory
> like ram_hi in that file: we allocate it and then don't care about
> freeing it, because we don't happen to have a board state struct.
> The correct fix if you care about this kind of thing would be
> to have a board state struct which had MemoryRegion fields (not
> MemoryRegion* fields). We have lots of bits of memory that we
> allocate once on startup and then don't care about freeing.
>

I think we are talking about a bit different problems here. Indeed
ram_hi is allocated, then used until QEMU exits but is never freed. Yet
it is never completely lost: there is at least one pointer to it in
memory hierarchy. Valgrind calls such situations "still reachable" and
does not consider them errors (because memory is in use until the very
moment the program exits; at least it can not be proven different).

ram_lo is different. It can be added to memory hierarchy in which case
it will behave exactly the same way as ram_hi. But it may be not used at
all in which case all pointers to it will be lost. This is the real
memory leak. Valgrind reports such situations as "definitely lost" and
they are considered errors (because it can be proven that memory was
allocated, is not in use and was not freed).

In our case ram_lo was reported as "definitely lost" while ram_hi was
"still reachable" and was never reported as error.

This patch addresses the second problem (when ram_lo is "definitely
lost") because it has very short and simple solution. While you are
arguing that we need to address the first problem - which is also true
but it is different problem that will need different solution and a much
larger one.

> It just
> doesn't seem to me very useful to merely silence the warning
> rather than actually fixing the underlying thing that the
> warning is telling you about.
> 

As I described above, it actually solves the problem Valgrind reports.
It is just a different problem than you are talking about.


> I'll probably put it in, because it's not very harmful.

Either way is fine with me. I'm still sure this patch is worthwhile but on
the other hand it is not that big of an issue to be arguing about it for
too long.

-- 
Kirill