[libvirt] [PATCH] qemu: unlock qemu driver before return
--- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 104e92d..6e3edde 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2068,12 +2068,14 @@ static int qemudDomainSave(virDomainPtr dom, const char *path) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Invalid save image format specified " "in configuration file")); + qemuDriverUnlock(driver); return -1; } if (!qemudCompressProgramAvailable(compressed)) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Compression program for image format " "in configuration file isn't available")); + qemuDriverUnlock(driver); return -1; } } -- 1.7.3.1
At 03/30/2011 09:49 AM, Hu Tao Write:
--- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 104e92d..6e3edde 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2068,12 +2068,14 @@ static int qemudDomainSave(virDomainPtr dom, const char *path) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Invalid save image format specified " "in configuration file")); + qemuDriverUnlock(driver); return -1; } if (!qemudCompressProgramAvailable(compressed)) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Compression program for image format " "in configuration file isn't available")); + qemuDriverUnlock(driver); return -1; } }
I checked all the place where calling qemuDriverLock(), and I found 2 similar problem: 1. qemudStartup(): ================= *qemuDriverUnlock(qemu_driver);* qemuAutostartDomains(qemu_driver); qemu_driver->workerPool = virThreadPoolNew(0, 1, processWatchdogEvent, qemu_driver); if (!qemu_driver->workerPool) goto error; if (conn) virConnectClose(conn); return 0; out_of_memory: virReportOOMError(); error: if (qemu_driver) *qemuDriverUnlock(qemu_driver);* ================= We unlock qemu_driver twice when virThreadPoolNew() failed. I think we should lock qemu_driver again before calling virThreadPoolNew(). 2. processWatchdogEvent(): ================= switch (wdEvent->action) { case VIR_DOMAIN_WATCHDOG_ACTION_DUMP: { .... qemuDriverLock(driver); virDomainObjLock(wdEvent->vm); if (qemuDomainObjBeginJobWithDriver(driver, wdEvent->vm) < 0) break; if (!virDomainObjIsActive(wdEvent->vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); break; } ================= We lock qemu_driver and vm, but we do not unlock them when qemuDomainObjBeginJobWithDriver() failed or the vm is not running.
When I checked all the place where calling qemuDriverLock(), I found some problem. The details of the problem is here: https://www.redhat.com/archives/libvir-list/2011-March/msg01409.html When I modify the function processWatchdogEvent(), I found that wdEvent->vm may be freed before processWatchdogEvent() is called. Wen Congyang (3): qemu: avoid qemu_driver being unlocked twice when virThreadPoolNew() failed qemu: unlock qemu driver and vm before returning from processWatchdogEvent() hold an extra reference while handling watchdog event src/qemu/qemu_driver.c | 34 +++++++++++++++++++++++++--------- src/qemu/qemu_process.c | 4 ++++ 2 files changed, 29 insertions(+), 9 deletions(-)
--- 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 f843dc8..d79d61b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2403,19 +2403,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, @@ -2432,16 +2435,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 Wed, Mar 30, 2011 at 12:34:39PM +0800, Wen Congyang wrote:
--- 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 f843dc8..d79d61b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2403,19 +2403,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, @@ -2432,16 +2435,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); }
Looks good to me.
--- src/qemu/qemu_driver.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd12dc8..f843dc8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -628,10 +628,14 @@ qemudStartup(int privileged) { qemuAutostartDomains(qemu_driver); + /* Lock qemu_drive again as we will modify it */ + qemuDriverLock(qemu_driver); qemu_driver->workerPool = virThreadPoolNew(0, 1, processWatchdogEvent, qemu_driver); if (!qemu_driver->workerPool) goto error; + qemuDriverUnlock(qemu_driver); + if (conn) virConnectClose(conn); -- 1.7.1
On Wed, Mar 30, 2011 at 12:34:47PM +0800, Wen Congyang wrote:
--- src/qemu/qemu_driver.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd12dc8..f843dc8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -628,10 +628,14 @@ qemudStartup(int privileged) {
qemuAutostartDomains(qemu_driver);
+ /* Lock qemu_drive again as we will modify it */ + qemuDriverLock(qemu_driver); qemu_driver->workerPool = virThreadPoolNew(0, 1, processWatchdogEvent, qemu_driver); if (!qemu_driver->workerPool) goto error;
+ qemuDriverUnlock(qemu_driver); + if (conn) virConnectClose(conn);
-- 1.7.1
Looks strange although it's right, What about this? diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 104e92d..91432ed 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);
At 03/30/2011 01:48 PM, Hu Tao Write:
On Wed, Mar 30, 2011 at 12:34:47PM +0800, Wen Congyang wrote:
--- src/qemu/qemu_driver.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd12dc8..f843dc8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -628,10 +628,14 @@ qemudStartup(int privileged) {
qemuAutostartDomains(qemu_driver);
+ /* Lock qemu_drive again as we will modify it */ + qemuDriverLock(qemu_driver); qemu_driver->workerPool = virThreadPoolNew(0, 1, processWatchdogEvent, qemu_driver); if (!qemu_driver->workerPool) goto error;
+ qemuDriverUnlock(qemu_driver); + if (conn) virConnectClose(conn);
-- 1.7.1
Looks strange although it's right, What about this?
Hmm, we must create workerPool before auto starting domains, because we may use this pool during auto starting domains. I will send v2.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 104e92d..91432ed 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);
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 | 10 ++++++---- src/qemu/qemu_process.c | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d79d61b..c9c681f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2443,15 +2443,17 @@ static void processWatchdogEvent(void *data, void *opaque) } 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) - virDomainObjUnlock(wdEvent->vm); qemuDriverUnlock(driver); cleanup: + if (virDomainObjUnref(wdEvent->vm) > 0) + virDomainObjUnlock(wdEvent->vm); VIR_FREE(wdEvent); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e31e1b4..cd8c726 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 Wed, Mar 30, 2011 at 12:34:49PM +0800, Wen Congyang wrote:
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 | 10 ++++++---- src/qemu/qemu_process.c | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d79d61b..c9c681f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2443,15 +2443,17 @@ static void processWatchdogEvent(void *data, void *opaque) }
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) - virDomainObjUnlock(wdEvent->vm); qemuDriverUnlock(driver);
cleanup: + if (virDomainObjUnref(wdEvent->vm) > 0) + virDomainObjUnlock(wdEvent->vm);
These two lines should be protected by qemu driver lock.
VIR_FREE(wdEvent); }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e31e1b4..cd8c726 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
At 03/30/2011 02:05 PM, Hu Tao Write:
On Wed, Mar 30, 2011 at 12:34:49PM +0800, Wen Congyang wrote:
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 | 10 ++++++---- src/qemu/qemu_process.c | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d79d61b..c9c681f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2443,15 +2443,17 @@ static void processWatchdogEvent(void *data, void *opaque) }
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) - virDomainObjUnlock(wdEvent->vm); qemuDriverUnlock(driver);
cleanup: + if (virDomainObjUnref(wdEvent->vm) > 0) + virDomainObjUnlock(wdEvent->vm);
These two lines should be protected by qemu driver lock.
We should not unlock domain here as we reached here without locking domain. Will update this patch.
VIR_FREE(wdEvent); }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e31e1b4..cd8c726 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 Wed, Mar 30, 2011 at 09:49:29AM +0800, Hu Tao wrote:
--- src/qemu/qemu_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 104e92d..6e3edde 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2068,12 +2068,14 @@ static int qemudDomainSave(virDomainPtr dom, const char *path) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Invalid save image format specified " "in configuration file")); + qemuDriverUnlock(driver); return -1; } if (!qemudCompressProgramAvailable(compressed)) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Compression program for image format " "in configuration file isn't available")); + qemuDriverUnlock(driver); return -1; } }
Whoops ! A severe problem ! Though I think the best way to do this is to "goto cleanup" instead of returning directly I just pushed the patch, thanks a lot ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel Veillard -
Hu Tao -
Wen Congyang