--- 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); }

--- 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: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