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

[Marc Hartmayer]

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(-)
>>
>> 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