[libvirt] [PATCH 1/3] qemu: avoid qemu_driver being unlocked twice when virThreadPoolNew() failed
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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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.
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 :|
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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
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 :|
于 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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 :)
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 :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Wen Congyang -
Wen Congyang