On Mon, Feb 05, 2018 at 11:31 AM +0100, Michal Privoznik <mprivozn@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@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@linux.vnet.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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