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

[Michal Privoznik]

On 02/05/2018 12:32 PM, Marc Hartmayer wrote:
> 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.

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