On 02/05/2018 12:32 PM, Marc Hartmayer wrote:
> 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.
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
