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> 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> 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>
>>>> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
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