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

On 02/05/2018 12:32 PM, Marc Hartmayer wrote: > On Mon, Feb 05, 2018 at 11:31 AM +0100, Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: >> On 02/05/2018 11:17 AM, Marc Hartmayer wrote: >>> On Mon, Feb 05, 2018 at 10:35 AM +0100, Michal Privoznik <mprivozn(a)redhat.com&gt; 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(a)linux.vnet.ibm.com&gt; >>>>> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; >>>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; >>>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >>>>> --- >>>>> 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. Does it? After the first line it's @event 'object' (or struct or whatever it is) who holds the extra reference to @vm. So it makes sense that free(event) should remove that reference. In fact, event->vm = virObjectRef(vm); if (func() < 0) { virObjectUnref(vm); goto error; } error: qemuProcessEventFree(event); looks kind of wrong too because of the reason described earlier. IOW, if @vm wasn't an object but an allocated piece of memory we would never do: ALLOC(event->vm); if(func() < 0) { VIR_FREE(event->vm); goto error; } error: qemuProcessEventFree(event); but we would rather rely on qemuProcessEventFree freeing event->vm too. > Another way would be to convert > qemuProcessEvent to our object model. I don't think we need this. And even if we did, the dispose() function would need to unref the event->vm anyway (so we'd end up with basically the same code except of ALLOC()+free() we'd do ObjectNew() and ObjectUnref()).

Michal