On Mon, Feb 05, 2018 at 11:31 AM +0100, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote: > On 02/05/2018 11:17 AM, Marc Hartmayer wrote: >> On Mon, Feb 05, 2018 at 10:35 AM +0100, Michal Privoznik <mprivozn@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> >>>> Signed-off-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx> >>>> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxxxxxxx> >>>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx> >>>> --- >>>> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
