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(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index c8123ce59bc4..4472b00d6540 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -10910,3 +10910,26 @@ qemuDomainPrepareDiskSource(virConnectPtr conn,
>>
>> return 0;
>> }
>> +
>> +
>> +void
>> +qemuProcessEventFree(struct qemuProcessEvent *event)
>> +{
>> + if (!event)
>> + return;
>> +
>> + switch (event->eventType) {
>> + case QEMU_PROCESS_EVENT_GUESTPANIC:
>> + qemuMonitorEventPanicInfoFree(event->data);
>> + break;
>> + case QEMU_PROCESS_EVENT_WATCHDOG:
>> + case QEMU_PROCESS_EVENT_DEVICE_DELETED:
>> + case QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED:
>> + case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
>> + case QEMU_PROCESS_EVENT_BLOCK_JOB:
>> + case QEMU_PROCESS_EVENT_MONITOR_EOF:
>> + case QEMU_PROCESS_EVENT_LAST:
>> + VIR_FREE(event->data);
>
> We should take EVENT_LAST to a separate block.
Makes sense.
>
>> + }
>> + VIR_FREE(event);
>> +}
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index ddfc46dcd0c1..7c9364f0bb69 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -445,6 +445,8 @@ struct qemuProcessEvent {
>> void *data;
>> };
>>
>> +void qemuProcessEventFree(struct qemuProcessEvent *event);
>> +
>> typedef struct _qemuDomainLogContext qemuDomainLogContext;
>> typedef qemuDomainLogContext *qemuDomainLogContextPtr;
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d64686df4c5f..d760b77c81e7 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4183,7 +4183,6 @@ processWatchdogEvent(virQEMUDriverPtr driver,
>> qemuDomainObjEndAsyncJob(driver, vm);
>>
>> cleanup:
>> - VIR_FREE(dumpfile);
>> virObjectUnref(cfg);
>
> No. @dumpfile is not taken from qemuProcessEvent rather than allocated
> in this function. This VIR_FREE() needs to stay.
Right.
>
>> }
>>
>> @@ -4309,7 +4308,6 @@ processGuestPanicEvent(virQEMUDriverPtr driver,
>> qemuDomainRemoveInactiveJob(driver, vm);
>>
>> cleanup:
>> - qemuMonitorEventPanicInfoFree(info);
>> virObjectUnref(cfg);
>> }
>>
>> @@ -4351,7 +4349,6 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
>> qemuDomainObjEndJob(driver, vm);
>>
>> cleanup:
>> - VIR_FREE(devAlias);
>
> This one is correct though. BTW: Now we can mark all these @devAlias
> arguments as 'const' to express it explicitly that we don't want these
> functions to free it.
Yep, good idea.
>
> 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?
>
> 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
