2:53 a.m.
We do not lock qemu_driver when calling virThreadPoolNew(). If it failed,
we will unlock qemu_driver. It is dangerous.
We may use this pool during auto starting domains. So we must create it before
calling qemuAutostartDomains(). Otherwise, libvirtd will crash.
---
src/qemu/qemu_driver.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 48fe266..dd84f65 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -624,14 +624,14 @@ qemudStartup(int privileged) {
virHashForEach(qemu_driver->domains.objs, qemuDomainSnapshotLoad,
qemu_driver->snapshotDir);
- qemuDriverUnlock(qemu_driver);
-
- qemuAutostartDomains(qemu_driver);
-
qemu_driver->workerPool = virThreadPoolNew(0, 1, processWatchdogEvent,
qemu_driver);
if (!qemu_driver->workerPool)
goto error;
+ qemuDriverUnlock(qemu_driver);
+
+ qemuAutostartDomains(qemu_driver);
+
if (conn)
virConnectClose(conn);
--
1.7.1
2:57 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: unlock qemu driver and vm before returning from processWatchdogEvent()
When the function processWatchdogEvent() failed, we only free wdEvent, but
forget to unlock qemu driver and vm, free dumpfile.
---
src/qemu/qemu_driver.c | 28 +++++++++++++++++++---------
1 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dd84f65..628cfe3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2400,19 +2400,22 @@ static void processWatchdogEvent(void *data, void *opaque)
wdEvent->vm->def->name,
(unsigned int)time(NULL)) < 0) {
virReportOOMError();
- break;
+ goto cleanup;
}
qemuDriverLock(driver);
virDomainObjLock(wdEvent->vm);
- if (qemuDomainObjBeginJobWithDriver(driver, wdEvent->vm) < 0)
- break;
+ if (qemuDomainObjBeginJobWithDriver(driver, wdEvent->vm) < 0) {
+ VIR_FREE(dumpfile);
+ goto unlock;
+ }
if (!virDomainObjIsActive(wdEvent->vm)) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("domain is not running"));
- break;
+ VIR_FREE(dumpfile);
+ goto endjob;
}
ret = doCoreDump(driver,
@@ -2429,16 +2432,23 @@ static void processWatchdogEvent(void *data, void *opaque)
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("Resuming after dump
failed"));
- if (qemuDomainObjEndJob(wdEvent->vm) > 0)
- virDomainObjUnlock(wdEvent->vm);
-
- qemuDriverUnlock(driver);
-
VIR_FREE(dumpfile);
}
break;
+ default:
+ goto cleanup;
}
+endjob:
+ if (qemuDomainObjEndJob(wdEvent->vm) == 0)
+ wdEvent->vm = NULL;
+
+unlock:
+ if (wdEvent->vm)
+ virDomainObjUnlock(wdEvent->vm);
+ qemuDriverUnlock(driver);
+
+cleanup:
VIR_FREE(wdEvent);
}
--
1.7.1
3:34 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: unlock qemu driver and vm before returning from processWatchdogEvent()
On 04/06/2011 01:57 AM, Wen Congyang wrote:
When the function processWatchdogEvent() failed, we only free
wdEvent, but
forget to unlock qemu driver and vm, free dumpfile.
---
src/qemu/qemu_driver.c | 28 +++++++++++++++++++---------
1 files changed, 19 insertions(+), 9 deletions(-)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:45 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: unlock qemu driver and vm before returning from processWatchdogEvent()
On 04/06/2011 01:57 AM, Wen Congyang wrote:
When the function processWatchdogEvent() failed, we only free
wdEvent, but
forget to unlock qemu driver and vm, free dumpfile.
---
src/qemu/qemu_driver.c | 28 +++++++++++++++++++---------
1 files changed, 19 insertions(+), 9 deletions(-)
I spoke too soon.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dd84f65..628cfe3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2400,19 +2400,22 @@ static void processWatchdogEvent(void *data, void *opaque)
wdEvent->vm->def->name,
Oops - this is reading from wdEvent->vm contents...
(unsigned int)time(NULL)) < 0) {
virReportOOMError();
- break;
+ goto cleanup;
}
qemuDriverLock(driver);
virDomainObjLock(wdEvent->vm);
...prior to obtaining the lock. I think we need to swap that order.
+ default:
+ goto cleanup;
}
+endjob:
+ if (qemuDomainObjEndJob(wdEvent->vm) == 0)
+ wdEvent->vm = NULL;
+
+unlock:
+ if (wdEvent->vm)
+ virDomainObjUnlock(wdEvent->vm);
+ qemuDriverUnlock(driver);
+
+cleanup:
VIR_FREE(wdEvent);
If you do that, then nothing in the VIR_DOMAIN_WATCHDOG_ACTION_DUMP case
jumps to cleanup, and after patch 3, the default case also unlocks the
vm rather than jumping to cleanup. I think you need to send a v2 patch
with 2 and 3 squashed into a single fix.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:11 p.m.
New subject: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
This patch do the following two things:
1. hold an extra reference while handling watchdog event
If the domain is not persistent, and qemu quits unexpectedly before
calling processWatchdogEvent(), vm will be freed and the function
processWatchdogEvent() will be dangerous.
2. unlock qemu driver and vm before returning from processWatchdogEvent()
When the function processWatchdogEvent() failed, we only free wdEvent,
but forget to unlock qemu driver and vm, free dumpfile.
---
src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------
src/qemu/qemu_process.c | 4 ++++
2 files changed, 26 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d5c1274..f6e503a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2442,6 +2442,9 @@ static void processWatchdogEvent(void *data, void *opaque)
struct qemuDomainWatchdogEvent *wdEvent = data;
struct qemud_driver *driver = opaque;
+ qemuDriverLock(driver);
+ virDomainObjLock(wdEvent->vm);
+
switch (wdEvent->action) {
case VIR_DOMAIN_WATCHDOG_ACTION_DUMP:
{
@@ -2452,19 +2455,19 @@ static void processWatchdogEvent(void *data, void *opaque)
wdEvent->vm->def->name,
(unsigned int)time(NULL)) < 0) {
virReportOOMError();
- break;
+ goto unlock;
}
- qemuDriverLock(driver);
- virDomainObjLock(wdEvent->vm);
-
- if (qemuDomainObjBeginJobWithDriver(driver, wdEvent->vm) < 0)
- break;
+ if (qemuDomainObjBeginJobWithDriver(driver, wdEvent->vm) < 0) {
+ VIR_FREE(dumpfile);
+ goto unlock;
+ }
if (!virDomainObjIsActive(wdEvent->vm)) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("domain is not running"));
- break;
+ VIR_FREE(dumpfile);
+ goto endjob;
}
ret = doCoreDump(driver,
@@ -2481,16 +2484,23 @@ static void processWatchdogEvent(void *data, void *opaque)
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("Resuming after dump
failed"));
- if (qemuDomainObjEndJob(wdEvent->vm) > 0)
- virDomainObjUnlock(wdEvent->vm);
-
- qemuDriverUnlock(driver);
-
VIR_FREE(dumpfile);
}
break;
+ default:
+ goto unlock;
}
+endjob:
+ /* Safe to ignore value since ref count was incremented in
+ * qemuProcessHandleWatchdog().
+ */
+ ignore_value(qemuDomainObjEndJob(wdEvent->vm));
+
+unlock:
+ if (virDomainObjUnref(wdEvent->vm) > 0)
+ virDomainObjUnlock(wdEvent->vm);
+ qemuDriverUnlock(driver);
VIR_FREE(wdEvent);
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7295f9e..b6bec46 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -428,6 +428,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (VIR_ALLOC(wdEvent) == 0) {
wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
wdEvent->vm = vm;
+ /* Hold an extra reference because we can't allow 'vm' to be
+ * deleted before handling watchdog event is finished.
+ */
+ virDomainObjRef(vm);
ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
} else
virReportOOMError();
--
1.7.1
11:36 a.m.
New subject: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
On 04/14/2011 09:11 PM, Wen Congyang wrote:
This patch do the following two things:
s/do/does/
1. hold an extra reference while handling watchdog event
If the domain is not persistent, and qemu quits unexpectedly before
calling processWatchdogEvent(), vm will be freed and the function
processWatchdogEvent() will be dangerous.
2. unlock qemu driver and vm before returning from processWatchdogEvent()
When the function processWatchdogEvent() failed, we only free wdEvent,
but forget to unlock qemu driver and vm, free dumpfile.
---
src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------
src/qemu/qemu_process.c | 4 ++++
2 files changed, 26 insertions(+), 12 deletions(-)
Looks like your v2 caught my review comments correctly. But I found one
more issue:
+++ b/src/qemu/qemu_process.c
@@ -428,6 +428,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (VIR_ALLOC(wdEvent) == 0) {
wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
wdEvent->vm = vm;
+ /* Hold an extra reference because we can't allow 'vm' to be
+ * deleted before handling watchdog event is finished.
+ */
+ virDomainObjRef(vm);
ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
Now that we have increased the ref count, we should decrease it if we
are unable to send a job to the thread pool. That is, replace the
ignore_value() with:
if (virThreadPoolSendJob(...) < 0) {
virDomainObjUnref(vm);
VIR_FREE(wdEvent);
}
ACK with that change squashed in.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:41 p.m.
New subject: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
At 04/16/2011 12:36 AM, Eric Blake Write:
On 04/14/2011 09:11 PM, Wen Congyang wrote:
> This patch do the following two things:
s/do/does/
> 1. hold an extra reference while handling watchdog event
> If the domain is not persistent, and qemu quits unexpectedly before
> calling processWatchdogEvent(), vm will be freed and the function
> processWatchdogEvent() will be dangerous.
>
> 2. unlock qemu driver and vm before returning from processWatchdogEvent()
> When the function processWatchdogEvent() failed, we only free wdEvent,
> but forget to unlock qemu driver and vm, free dumpfile.
>
>
> ---
> src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------
> src/qemu/qemu_process.c | 4 ++++
> 2 files changed, 26 insertions(+), 12 deletions(-)
Looks like your v2 caught my review comments correctly. But I found one
more issue:
> +++ b/src/qemu/qemu_process.c
> @@ -428,6 +428,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> if (VIR_ALLOC(wdEvent) == 0) {
> wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
> wdEvent->vm = vm;
> + /* Hold an extra reference because we can't allow 'vm' to
be
> + * deleted before handling watchdog event is finished.
> + */
> + virDomainObjRef(vm);
> ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
Now that we have increased the ref count, we should decrease it if we
are unable to send a job to the thread pool. That is, replace the
ignore_value() with:
if (virThreadPoolSendJob(...) < 0) {
virDomainObjUnref(vm);
VIR_FREE(wdEvent);
}
ACK with that change squashed in.
I have pushed this series patch with this chaged squashed in.
Thanks for reviewing.
6:41 a.m.
New subject: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
On Fri, Apr 15, 2011 at 10:36:10AM -0600, Eric Blake wrote:
On 04/14/2011 09:11 PM, Wen Congyang wrote:
> This patch do the following two things:
s/do/does/
> 1. hold an extra reference while handling watchdog event
> If the domain is not persistent, and qemu quits unexpectedly before
> calling processWatchdogEvent(), vm will be freed and the function
> processWatchdogEvent() will be dangerous.
>
> 2. unlock qemu driver and vm before returning from processWatchdogEvent()
> When the function processWatchdogEvent() failed, we only free wdEvent,
> but forget to unlock qemu driver and vm, free dumpfile.
>
>
> ---
> src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------
> src/qemu/qemu_process.c | 4 ++++
> 2 files changed, 26 insertions(+), 12 deletions(-)
Looks like your v2 caught my review comments correctly. But I found one
more issue:
> +++ b/src/qemu/qemu_process.c
> @@ -428,6 +428,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> if (VIR_ALLOC(wdEvent) == 0) {
> wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
> wdEvent->vm = vm;
> + /* Hold an extra reference because we can't allow 'vm' to
be
> + * deleted before handling watchdog event is finished.
> + */
> + virDomainObjRef(vm);
> ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
Now that we have increased the ref count, we should decrease it if we
are unable to send a job to the thread pool. That is, replace the
ignore_value() with:
if (virThreadPoolSendJob(...) < 0) {
virDomainObjUnref(vm);
VIR_FREE(wdEvent);
}
ACK with that change squashed in.
This last minute addition caused a build failure
cc1: warnings being treated as errors
qemu/qemu_process.c: In function 'qemuProcessHandleWatchdog':
qemu/qemu_process.c:436:34: error: ignoring return value of 'virDomainObjUnref',
declared with attribute warn_unused_result [-Wunused-result]
make[3]: *** [libvirt_driver_qemu_la-qemu_process.lo] Error 1
I think we also need this added:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d405dda..5a81265 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -433,14 +433,16 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
*/
virDomainObjRef(vm);
if (virThreadPoolSendJob(driver->workerPool, wdEvent) < 0) {
- virDomainObjUnref(vm);
+ if (virDomainObjUnref(vm) < 0)
+ vm = NULL;
VIR_FREE(wdEvent);
}
} else
virReportOOMError();
}
- virDomainObjUnlock(vm);
+ if (vm)
+ virDomainObjUnlock(vm);
if (watchdogEvent || lifecycleEvent) {
qemuDriverLock(driver);
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:15 a.m.
New subject: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
On 04/18/2011 05:41 AM, Daniel P. Berrange wrote:
> Looks like your v2 caught my review comments correctly. But I
found one
> more issue:
>
> Now that we have increased the ref count, we should decrease it if we
> are unable to send a job to the thread pool. That is, replace the
> ignore_value() with:
>
> if (virThreadPoolSendJob(...) < 0) {
> virDomainObjUnref(vm);
> VIR_FREE(wdEvent);
> }
>
> ACK with that change squashed in.
This last minute addition caused a build failure
Oh well - that's what we get for incorporating something that I just
typed, rather than testing...
I think we also need this added:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d405dda..5a81265 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -433,14 +433,16 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
*/
virDomainObjRef(vm);
if (virThreadPoolSendJob(driver->workerPool, wdEvent) < 0) {
- virDomainObjUnref(vm);
+ if (virDomainObjUnref(vm) < 0)
+ vm = NULL;
VIR_FREE(wdEvent);
}
} else
virReportOOMError();
}
While we're at it, let's fix this else branch to have proper {} usage,
per HACKING style guidelines.
- virDomainObjUnlock(vm);
+ if (vm)
+ virDomainObjUnlock(vm);
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:32 a.m.
New subject: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
On 04/18/2011 09:15 AM, Eric Blake wrote:
> I think we also need this added:
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d405dda..5a81265 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -433,14 +433,16 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> */
> virDomainObjRef(vm);
> if (virThreadPoolSendJob(driver->workerPool, wdEvent) < 0) {
> - virDomainObjUnref(vm);
> + if (virDomainObjUnref(vm) < 0)
> + vm = NULL;
> VIR_FREE(wdEvent);
> }
> } else
> virReportOOMError();
> }
While we're at it, let's fix this else branch to have proper {} usage,
per HACKING style guidelines.
>
> - virDomainObjUnlock(vm);
> + if (vm)
> + virDomainObjUnlock(vm);
ACK.
I've gone ahead and pushed this under your name, under the build-breaker
rule.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:01 p.m.
New subject: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()
At 04/18/2011 07:41 PM, Daniel P. Berrange Write:
On Fri, Apr 15, 2011 at 10:36:10AM -0600, Eric Blake wrote:
> On 04/14/2011 09:11 PM, Wen Congyang wrote:
>> This patch do the following two things:
>
> s/do/does/
>
>> 1. hold an extra reference while handling watchdog event
>> If the domain is not persistent, and qemu quits unexpectedly before
>> calling processWatchdogEvent(), vm will be freed and the function
>> processWatchdogEvent() will be dangerous.
>>
>> 2. unlock qemu driver and vm before returning from processWatchdogEvent()
>> When the function processWatchdogEvent() failed, we only free wdEvent,
>> but forget to unlock qemu driver and vm, free dumpfile.
>>
>>
>> ---
>> src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------
>> src/qemu/qemu_process.c | 4 ++++
>> 2 files changed, 26 insertions(+), 12 deletions(-)
>
> Looks like your v2 caught my review comments correctly. But I found one
> more issue:
>
>> +++ b/src/qemu/qemu_process.c
>> @@ -428,6 +428,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
>> if (VIR_ALLOC(wdEvent) == 0) {
>> wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
>> wdEvent->vm = vm;
>> + /* Hold an extra reference because we can't allow 'vm'
to be
>> + * deleted before handling watchdog event is finished.
>> + */
>> + virDomainObjRef(vm);
>> ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
>
> Now that we have increased the ref count, we should decrease it if we
> are unable to send a job to the thread pool. That is, replace the
> ignore_value() with:
>
> if (virThreadPoolSendJob(...) < 0) {
> virDomainObjUnref(vm);
> VIR_FREE(wdEvent);
> }
>
> ACK with that change squashed in.
This last minute addition caused a build failure
Sorry for introducing a build failure.
I have tested building before pushing this patch. But I did not meet any error, and
I do not notice the warning.
The default CFLAGS does not include '-Werror'.
cc1: warnings being treated as errors
qemu/qemu_process.c: In function 'qemuProcessHandleWatchdog':
qemu/qemu_process.c:436:34: error: ignoring return value of 'virDomainObjUnref',
declared with attribute warn_unused_result [-Wunused-result]
make[3]: *** [libvirt_driver_qemu_la-qemu_process.lo] Error 1
I think we also need this added:
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d405dda..5a81265 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -433,14 +433,16 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
*/
virDomainObjRef(vm);
if (virThreadPoolSendJob(driver->workerPool, wdEvent) < 0) {
- virDomainObjUnref(vm);
+ if (virDomainObjUnref(vm) < 0)
+ vm = NULL;
The return value of virDomainObjUnref() should not less than 0.
If vm is unlocked in virDomainObjUnref(), the return value is 0.
It is safe to check the return value here, but it should not happen,
as we have held an extra reference when opening the monitor.
This patch has been pushed, so I will post another patch to fix the problem.
VIR_FREE(wdEvent);
}
} else
virReportOOMError();
}
- virDomainObjUnlock(vm);
+ if (vm)
+ virDomainObjUnlock(vm);
if (watchdogEvent || lifecycleEvent) {
qemuDriverLock(driver);
Daniel
9:08 p.m.
New subject: [libvirt] [PATCH] qemu: fix the check of virDomainObjUnref()'s return value
If vm is unlocked in virDomainObjUnref(), the return value is 0, not less
than 0.
---
src/qemu/qemu_process.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1dfd005..7691cbe 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -433,7 +433,7 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
*/
virDomainObjRef(vm);
if (virThreadPoolSendJob(driver->workerPool, wdEvent) < 0) {
- if (virDomainObjUnref(vm) < 0)
+ if (virDomainObjUnref(vm) == 0)
vm = NULL;
VIR_FREE(wdEvent);
}
--
1.7.1
4:34 a.m.
New subject: [libvirt] [PATCH] qemu: fix the check of virDomainObjUnref()'s return value
On Tue, Apr 19, 2011 at 10:08:21AM +0800, Wen Congyang wrote:
If vm is unlocked in virDomainObjUnref(), the return value is 0, not
less
than 0.
---
src/qemu/qemu_process.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1dfd005..7691cbe 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -433,7 +433,7 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
*/
virDomainObjRef(vm);
if (virThreadPoolSendJob(driver->workerPool, wdEvent) < 0) {
- if (virDomainObjUnref(vm) < 0)
+ if (virDomainObjUnref(vm) == 0)
vm = NULL;
VIR_FREE(wdEvent);
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8:49 a.m.
New subject: [libvirt] [PATCH] qemu: fix the check of virDomainObjUnref()'s return value
于 2011-4-19 17:34, Daniel P. Berrange 写道:
On Tue, Apr 19, 2011 at 10:08:21AM +0800, Wen Congyang wrote:
> If vm is unlocked in virDomainObjUnref(), the return value is 0, not less
> than 0.
>
> ---
> src/qemu/qemu_process.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1dfd005..7691cbe 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -433,7 +433,7 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> */
> virDomainObjRef(vm);
> if (virThreadPoolSendJob(driver->workerPool, wdEvent)< 0) {
> - if (virDomainObjUnref(vm)< 0)
> + if (virDomainObjUnref(vm) == 0)
> vm = NULL;
> VIR_FREE(wdEvent);
ACK
Thanks, pushed.
Daniel
2:58 a.m.
New subject: [libvirt] [PATCH V2 3/3] hold an extra reference while handling watchdog event
If the domain is not persistent, and qemu quited unexpectedly before
calling processWatchdogEvent(), vm will be freed and the function
processWatchdogEvent() will be dangerous.
---
src/qemu/qemu_driver.c | 12 ++++++++----
src/qemu/qemu_process.c | 4 ++++
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 628cfe3..d02891c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2436,15 +2436,19 @@ static void processWatchdogEvent(void *data, void *opaque)
}
break;
default:
- goto cleanup;
+ qemuDriverLock(driver);
+ virDomainObjLock(wdEvent->vm);
+ goto unlock;
}
endjob:
- if (qemuDomainObjEndJob(wdEvent->vm) == 0)
- wdEvent->vm = NULL;
+ /* Safe to ignore value since ref count was incremented in
+ * qemuProcessHandleWatchdog().
+ */
+ ignore_value(qemuDomainObjEndJob(wdEvent->vm));
unlock:
- if (wdEvent->vm)
+ if (virDomainObjUnref(wdEvent->vm) > 0)
virDomainObjUnlock(wdEvent->vm);
qemuDriverUnlock(driver);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 48ecd5c..321374c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -426,6 +426,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (VIR_ALLOC(wdEvent) == 0) {
wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
wdEvent->vm = vm;
+ /* Hold an extra reference because we can't allow 'vm' to be
+ * deleted before handling watchdog event is finished.
+ */
+ virDomainObjRef(vm);
ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
} else
virReportOOMError();
--
1.7.1
3:43 p.m.
New subject: [libvirt] [PATCH V2 3/3] hold an extra reference while handling watchdog event
On 04/06/2011 01:58 AM, Wen Congyang wrote:
If the domain is not persistent, and qemu quited unexpectedly before
s/quited/quits/
calling processWatchdogEvent(), vm will be freed and the function
processWatchdogEvent() will be dangerous.
---
src/qemu/qemu_driver.c | 12 ++++++++----
src/qemu/qemu_process.c | 4 ++++
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 628cfe3..d02891c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2436,15 +2436,19 @@ static void processWatchdogEvent(void *data, void *opaque)
}
break;
default:
- goto cleanup;
+ qemuDriverLock(driver);
+ virDomainObjLock(wdEvent->vm);
+ goto unlock;
}
endjob:
- if (qemuDomainObjEndJob(wdEvent->vm) == 0)
- wdEvent->vm = NULL;
+ /* Safe to ignore value since ref count was incremented in
+ * qemuProcessHandleWatchdog().
+ */
+ ignore_value(qemuDomainObjEndJob(wdEvent->vm));
unlock:
- if (wdEvent->vm)
+ if (virDomainObjUnref(wdEvent->vm) > 0)
virDomainObjUnlock(wdEvent->vm);
qemuDriverUnlock(driver);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 48ecd5c..321374c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -426,6 +426,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
if (VIR_ALLOC(wdEvent) == 0) {
wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP;
wdEvent->vm = vm;
+ /* Hold an extra reference because we can't allow 'vm' to be
+ * deleted before handling watchdog event is finished.
+ */
+ virDomainObjRef(vm);
ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent));
ACK to the need to do this. Hmm, maybe we should just squash 2 and 3
together into one patch. Also, I found a flaw in 2 while reviewing this
one.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:53 a.m.
At 04/06/2011 03:53 PM, Wen Congyang Write:
We do not lock qemu_driver when calling virThreadPoolNew(). If it
failed,
we will unlock qemu_driver. It is dangerous.
We may use this pool during auto starting domains. So we must create it before
calling qemuAutostartDomains(). Otherwise, libvirtd will crash.
---
src/qemu/qemu_driver.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 48fe266..dd84f65 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -624,14 +624,14 @@ qemudStartup(int privileged) {
virHashForEach(qemu_driver->domains.objs, qemuDomainSnapshotLoad,
qemu_driver->snapshotDir);
- qemuDriverUnlock(qemu_driver);
-
- qemuAutostartDomains(qemu_driver);
-
qemu_driver->workerPool = virThreadPoolNew(0, 1, processWatchdogEvent,
qemu_driver);
if (!qemu_driver->workerPool)
goto error;
+ qemuDriverUnlock(qemu_driver);
+
+ qemuAutostartDomains(qemu_driver);
+
if (conn)
virConnectClose(conn);
Ping :)
5:21 a.m.
On Thu, Apr 14, 2011 at 01:53:19PM +0800, Wen Congyang wrote:
At 04/06/2011 03:53 PM, Wen Congyang Write:
> We do not lock qemu_driver when calling virThreadPoolNew(). If it failed,
> we will unlock qemu_driver. It is dangerous.
>
> We may use this pool during auto starting domains. So we must create it before
> calling qemuAutostartDomains(). Otherwise, libvirtd will crash.
>
> ---
> src/qemu/qemu_driver.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 48fe266..dd84f65 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -624,14 +624,14 @@ qemudStartup(int privileged) {
> virHashForEach(qemu_driver->domains.objs, qemuDomainSnapshotLoad,
> qemu_driver->snapshotDir);
>
> - qemuDriverUnlock(qemu_driver);
> -
> - qemuAutostartDomains(qemu_driver);
> -
> qemu_driver->workerPool = virThreadPoolNew(0, 1, processWatchdogEvent,
qemu_driver);
> if (!qemu_driver->workerPool)
> goto error;
>
> + qemuDriverUnlock(qemu_driver);
> +
> + qemuAutostartDomains(qemu_driver);
> +
> if (conn)
> virConnectClose(conn);
>
Ping :)
ACK
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5002
days inactive
5015
days old
17 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Wen Congyang -
Wen Congyang