[libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents

[Marc Hartmayer]

On Mon, Feb 05, 2018 at 11:31 AM +0100, Michal Privoznik <mprivozn at redhat.com> wrote:
> On 02/05/2018 11:17 AM, Marc Hartmayer wrote:
>> On Mon, Feb 05, 2018 at 10:35 AM +0100, Michal Privoznik <mprivozn at redhat.com> wrote:
>>> On 02/02/2018 01:13 PM, Marc Hartmayer wrote:
>>>> Add and use qemuProcessEventFree for freeing qemuProcessEvents. This
>>>> is less error-prone as the compiler can help us make sure that for
>>>> every new enumeration value of qemuProcessEventType the
>>>> qemuProcessEventFree function has to be adapted.
>>>>
>>>> All process*Event functions are *only* called by
>>>> qemuProcessHandleEvent and this function does the freeing by itself
>>>> with qemuProcessEventFree. This means that an explicit freeing of
>>>> processEvent->data is no longer required in each process*Event
>>>> handler.
>>>>
>>>> The effectiveness of this change is also demonstrated by the fact that
>>>> it fixes a memory leak of the panic info data in
>>>> qemuProcessHandleGuestPanic.
>>>>
>>>> Reported-by: Wang Dong <dongdwdw at linux.vnet.ibm.com>
>>>> Signed-off-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>>>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>>> ---
>>>>  src/qemu/qemu_domain.c  | 23 +++++++++++++++++++++++
>>>>  src/qemu/qemu_domain.h  |  2 ++
>>>>  src/qemu/qemu_driver.c  | 12 ++----------
>>>>  src/qemu/qemu_process.c | 22 +++++++---------------
>>>>  4 files changed, 34 insertions(+), 25 deletions(-)
>>>>
>
>
>>
>>>
>>> ACK with that changed.
>>>
>>> As a second step - should we move all those virObjectUnref(vm) calls
>>> into qemuProcessEventFree()? I mean those cases where
>>> virThreadPoolSendJob() fails and we call virObjectUnref(vm) followed by
>>> qemuProcessEventFree().
>>
>> Should work, but only if we can be sure that event->vm is always NULL
>> when no referencing has taken place. And I think we’ve to adapt the way
>> how for example qemuProcessHandleWatchdog is working (it uses the
>> information of virObjectUnref(vm)).
>>
>> But another question that came up to my mind: where happens the
>> unreferencing of the domain in the qemuProcessEventHandler? There is on
>> unreferencing in the virDomainObjEndAPI() call - is this the call?
>
> Yes. That's why I haven't made it in this patch but said it needs to be
> done separately. If we replace virDomainObjEndAPI() with just Unlock()
> and do the unref() in qemuProcessEventFree() it would look nicer IMO. I
> mean, visually it looks weird to have:

Yep. But:

event->vm = virObjectRef(vm);
...
error:
  qemuProcessEventFree(event);

This looks kind of confusing, too. Another way would be to convert
qemuProcessEvent to our object model.

>
> Lock();
> ...
> EndAPI(); // which does Unlock() + Unref()
>
> since we're not doing Ref() in this function.
>
> Michal
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294