1:13 p.m.
The second patch fixes the memory leak.
Marc Hartmayer (2):
qemu: Use the return value of virObjectRef directly
qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
src/qemu/qemu_domain.h | 2 ++
src/qemu/qemu_driver.c | 12 ++----------
src/qemu/qemu_process.c | 41 ++++++++++++++---------------------------
4 files changed, 41 insertions(+), 37 deletions(-)
--
2.13.4
1:13 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: Use the return value of virObjectRef directly
Use the return value of virObjectRef directly. This way, it's easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
---
src/qemu/qemu_process.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 5a364730c8c1..c17a6e9abfc6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -299,9 +299,8 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
goto cleanup;
processEvent->eventType = QEMU_PROCESS_EVENT_MONITOR_EOF;
- processEvent->vm = vm;
+ processEvent->vm = virObjectRef(vm);
- virObjectRef(vm);
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
ignore_value(virObjectUnref(vm));
VIR_FREE(processEvent);
@@ -911,11 +910,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (VIR_ALLOC(processEvent) == 0) {
processEvent->eventType = QEMU_PROCESS_EVENT_WATCHDOG;
processEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
- processEvent->vm = vm;
/* Hold an extra reference because we can't allow 'vm' to be
* deleted before handling watchdog event is finished.
*/
- virObjectRef(vm);
+ processEvent->vm = virObjectRef(vm);
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
if (!virObjectUnref(vm))
vm = NULL;
@@ -1349,12 +1347,12 @@ qemuProcessHandleGuestPanic(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
processEvent->eventType = QEMU_PROCESS_EVENT_GUESTPANIC;
processEvent->action = vm->def->onCrash;
- processEvent->vm = vm;
processEvent->data = info;
/* Hold an extra reference because we can't allow 'vm' to be
* deleted before handling guest panic event is finished.
*/
- virObjectRef(vm);
+ processEvent->vm = virObjectRef(vm);
+
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
if (!virObjectUnref(vm))
vm = NULL;
@@ -1395,9 +1393,8 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (VIR_STRDUP(data, devAlias) < 0)
goto error;
processEvent->data = data;
- processEvent->vm = vm;
+ processEvent->vm = virObjectRef(vm);
- virObjectRef(vm);
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
ignore_value(virObjectUnref(vm));
goto error;
@@ -1544,9 +1541,8 @@ qemuProcessHandleNicRxFilterChanged(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
if (VIR_STRDUP(data, devAlias) < 0)
goto error;
processEvent->data = data;
- processEvent->vm = vm;
+ processEvent->vm = virObjectRef(vm);
- virObjectRef(vm);
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
ignore_value(virObjectUnref(vm));
goto error;
@@ -1587,9 +1583,8 @@ qemuProcessHandleSerialChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
goto error;
processEvent->data = data;
processEvent->action = connected;
- processEvent->vm = vm;
+ processEvent->vm = virObjectRef(vm);
- virObjectRef(vm);
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
ignore_value(virObjectUnref(vm));
goto error;
--
2.13.4
10:35 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Use the return value of virObjectRef directly
On 02/02/2018 01:13 PM, Marc Hartmayer wrote:
Use the return value of virObjectRef directly. This way, it's
easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
---
src/qemu/qemu_process.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
Missed one:
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index c17a6e9ab..342339d5f 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -1033,11 +1033,10 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (VIR_STRDUP(data, diskAlias) < 0)
goto error;
processEvent->data = data;
- processEvent->vm = vm;
+ processEvent->vm = virObjectRef(vm);
processEvent->action = type;
processEvent->status = status;
- virObjectRef(vm);
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
ignore_value(virObjectUnref(vm));
goto error;
ACK with this squashed in.
Michal
1:13 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
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(a)linux.vnet.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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);
+ }
+ 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);
}
@@ -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);
virObjectUnref(cfg);
}
@@ -4648,7 +4645,6 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
cleanup:
virNetDevRxFilterFree(hostFilter);
virNetDevRxFilterFree(guestFilter);
- VIR_FREE(devAlias);
virObjectUnref(cfg);
}
@@ -4735,9 +4731,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
qemuDomainObjEndJob(driver, vm);
cleanup:
- VIR_FREE(devAlias);
virObjectUnref(cfg);
-
}
@@ -4751,7 +4745,7 @@ processBlockJobEvent(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk;
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
- goto cleanup;
+ return;
if (!virDomainObjIsActive(vm)) {
VIR_DEBUG("Domain is not running");
@@ -4763,8 +4757,6 @@ processBlockJobEvent(virQEMUDriverPtr driver,
endjob:
qemuDomainObjEndJob(driver, vm);
- cleanup:
- VIR_FREE(diskAlias);
}
@@ -4856,7 +4848,7 @@ static void qemuProcessEventHandler(void *data, void *opaque)
}
virDomainObjEndAPI(&vm);
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c17a6e9abfc6..f9b608434f47 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -303,7 +303,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
ignore_value(virObjectUnref(vm));
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
goto cleanup;
}
@@ -917,7 +917,7 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
if (!virObjectUnref(vm))
vm = NULL;
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
}
}
}
@@ -1048,9 +1048,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virObjectUnlock(vm);
return 0;
error:
- if (processEvent)
- VIR_FREE(processEvent->data);
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
goto cleanup;
}
@@ -1356,7 +1354,7 @@ qemuProcessHandleGuestPanic(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
if (!virObjectUnref(vm))
vm = NULL;
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
}
cleanup:
@@ -1404,9 +1402,7 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virObjectUnlock(vm);
return 0;
error:
- if (processEvent)
- VIR_FREE(processEvent->data);
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
goto cleanup;
}
@@ -1552,9 +1548,7 @@ qemuProcessHandleNicRxFilterChanged(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
virObjectUnlock(vm);
return 0;
error:
- if (processEvent)
- VIR_FREE(processEvent->data);
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
goto cleanup;
}
@@ -1594,9 +1588,7 @@ qemuProcessHandleSerialChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virObjectUnlock(vm);
return 0;
error:
- if (processEvent)
- VIR_FREE(processEvent->data);
- VIR_FREE(processEvent);
+ qemuProcessEventFree(processEvent);
goto cleanup;
}
--
2.13.4
10:35 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
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(a)linux.vnet.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
+ }
+ 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.
}
@@ -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.
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().
Michal
11:17 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
On Mon, Feb 05, 2018 at 10:35 AM +0100, Michal Privoznik <mprivozn(a)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(a)linux.vnet.ibm.com>
> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
11:31 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
On 02/05/2018 11:17 AM, Marc Hartmayer wrote:
On Mon, Feb 05, 2018 at 10:35 AM +0100, Michal Privoznik
<mprivozn(a)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(a)linux.vnet.ibm.com>
>> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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:
Lock();
...
EndAPI(); // which does Unlock() + Unref()
since we're not doing Ref() in this function.
Michal
12:32 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
On Mon, Feb 05, 2018 at 11:31 AM +0100, Michal Privoznik <mprivozn(a)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(a)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(a)linux.vnet.ibm.com>
>>> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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. Another way would be to convert
qemuProcessEvent to our object model.
Lock();
...
EndAPI(); // which does Unlock() + Unref()
since we're not doing Ref() in this function.
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
1:12 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
On 02/05/2018 12:32 PM, Marc Hartmayer wrote:
On Mon, Feb 05, 2018 at 11:31 AM +0100, Michal Privoznik
<mprivozn(a)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(a)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(a)linux.vnet.ibm.com>
>>>> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
4:06 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
On Mon, Feb 05, 2018 at 01:12 PM +0100, Michal Privoznik <mprivozn(a)redhat.com>
wrote:
On 02/05/2018 12:32 PM, Marc Hartmayer wrote:
> On Mon, Feb 05, 2018 at 11:31 AM +0100, Michal Privoznik <mprivozn(a)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(a)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(a)linux.vnet.ibm.com>
>>>>> Signed-off-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
>>>>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
>>>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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()).
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
10:35 a.m.
On 02/02/2018 01:13 PM, Marc Hartmayer wrote:
The second patch fixes the memory leak.
Marc Hartmayer (2):
qemu: Use the return value of virObjectRef directly
qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
src/qemu/qemu_domain.h | 2 ++
src/qemu/qemu_driver.c | 12 ++----------
src/qemu/qemu_process.c | 41 ++++++++++++++---------------------------
4 files changed, 41 insertions(+), 37 deletions(-)
I've fixed all the issues raised and pushed.
Thanks,
Michal
11:18 a.m.
On Mon, Feb 05, 2018 at 10:35 AM +0100, Michal Privoznik <mprivozn(a)redhat.com>
wrote:
On 02/02/2018 01:13 PM, Marc Hartmayer wrote:
> The second patch fixes the memory leak.
>
> Marc Hartmayer (2):
> qemu: Use the return value of virObjectRef directly
> qemu: Add and use qemuProcessEventFree for freeing qemuProcessEvents
>
> src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
> src/qemu/qemu_domain.h | 2 ++
> src/qemu/qemu_driver.c | 12 ++----------
> src/qemu/qemu_process.c | 41 ++++++++++++++---------------------------
> 4 files changed, 41 insertions(+), 37 deletions(-)
>
I've fixed all the issues raised and pushed.
Thanks!
Thanks,
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
2419
days inactive
2423
days old
11 comments
2 participants
participants (2)
-
Marc Hartmayer -
Michal Privoznik