On Mon, Feb 05, 2018 at 01:12 PM +0100, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
> 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()).
Yep, you’re right. The change would be useful.
>
> 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
