diff options
author | 2009-11-26 13:29:29 +0000 | |
---|---|---|
committer | 2009-12-11 16:01:56 +0100 | |
commit | 552c744f59ceb45d70150b348180c0f500124508 (patch) | |
tree | 20847fcf79dfbc1752526dd65a0e24a0302c5cfd | |
parent | Release of libvirt-0.7.4 (diff) | |
download | libvirt-552c744f59ceb45d70150b348180c0f500124508.tar.gz libvirt-552c744f59ceb45d70150b348180c0f500124508.tar.bz2 libvirt-552c744f59ceb45d70150b348180c0f500124508.zip |
Fix crash when deleting monitor while a command is in progress
If QEMU shuts down while we're in the middle of processing a
monitor command, the monitor will be freed, and upon cleaning
up we attempt to do qemuMonitorUnlock(priv->mon) when priv->mon
is NULL.
To address this we introduce proper reference counting into
the qemuMonitorPtr object, and hold an extra reference whenever
executing a command.
* src/qemu/qemu_driver.c: Hold a reference on the monitor while
executing commands, and only NULL-ify the priv->mon field when
the last reference is released
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Add reference
counting to handle safe deletion of monitor objects
-rw-r--r-- | src/qemu/qemu_driver.c | 35 | ||||
-rw-r--r-- | src/qemu/qemu_monitor.c | 84 | ||||
-rw-r--r-- | src/qemu/qemu_monitor.h | 5 |
3 files changed, 83 insertions, 41 deletions
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a4a87ac68..6a5344aca 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -274,6 +274,7 @@ static void qemuDomainObjEnterMonitor(virDomainObjPtr obj) qemuDomainObjPrivatePtr priv = obj->privateData; qemuMonitorLock(priv->mon); + qemuMonitorRef(priv->mon); virDomainObjUnlock(obj); } @@ -285,9 +286,19 @@ static void qemuDomainObjEnterMonitor(virDomainObjPtr obj) static void qemuDomainObjExitMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; + int refs; + + refs = qemuMonitorUnref(priv->mon); + + if (refs > 0) + qemuMonitorUnlock(priv->mon); - qemuMonitorUnlock(priv->mon); virDomainObjLock(obj); + + if (refs == 0) { + virDomainObjUnref(obj); + priv->mon = NULL; + } } @@ -304,6 +315,7 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, vir qemuDomainObjPrivatePtr priv = obj->privateData; qemuMonitorLock(priv->mon); + qemuMonitorRef(priv->mon); virDomainObjUnlock(obj); qemuDriverUnlock(driver); } @@ -317,10 +329,20 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, vir static void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; + int refs; + + refs = qemuMonitorUnref(priv->mon); + + if (refs > 0) + qemuMonitorUnlock(priv->mon); - qemuMonitorUnlock(priv->mon); qemuDriverLock(driver); virDomainObjLock(obj); + + if (refs == 0) { + virDomainObjUnref(obj); + priv->mon = NULL; + } } @@ -655,6 +677,10 @@ qemuConnectMonitor(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; + /* Hold an extra reference because we can't allow 'vm' to be + * deleted while the monitor is active */ + virDomainObjRef(vm); + if ((priv->mon = qemuMonitorOpen(vm, qemuHandleMonitorEOF)) == NULL) { VIR_ERROR(_("Failed to connect monitor for %s\n"), vm->def->name); return -1; @@ -2383,8 +2409,9 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, _("Failed to send SIGTERM to %s (%d)"), vm->def->name, vm->pid); - if (priv->mon) { - qemuMonitorClose(priv->mon); + if (priv->mon && + qemuMonitorClose(priv->mon) == 0) { + virDomainObjUnref(vm); priv->mon = NULL; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f0ef81b05..4cddd515e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -42,7 +42,7 @@ struct _qemuMonitor { virMutex lock; virCond notify; - virDomainObjPtr dom; + int refs; int fd; int watch; @@ -67,12 +67,14 @@ struct _qemuMonitor { * the next monitor msg */ int lastErrno; - /* If the monitor callback is currently active */ + /* If the monitor EOF callback is currently active (stops more commands being run) */ unsigned eofcb: 1; - /* If the monitor callback should free the closed monitor */ + /* If the monitor is in process of shutting down */ unsigned closed: 1; + }; + void qemuMonitorLock(qemuMonitorPtr mon) { virMutexLock(&mon->lock); @@ -84,21 +86,34 @@ void qemuMonitorUnlock(qemuMonitorPtr mon) } -static void qemuMonitorFree(qemuMonitorPtr mon, int lockDomain) +static void qemuMonitorFree(qemuMonitorPtr mon) { - VIR_DEBUG("mon=%p, lockDomain=%d", mon, lockDomain); - if (mon->vm) { - if (lockDomain) - virDomainObjLock(mon->vm); - if (!virDomainObjUnref(mon->vm) && lockDomain) - virDomainObjUnlock(mon->vm); - } + VIR_DEBUG("mon=%p", mon); if (virCondDestroy(&mon->notify) < 0) {} virMutexDestroy(&mon->lock); VIR_FREE(mon); } +int qemuMonitorRef(qemuMonitorPtr mon) +{ + mon->refs++; + return mon->refs; +} + +int qemuMonitorUnref(qemuMonitorPtr mon) +{ + mon->refs--; + + if (mon->refs == 0) { + qemuMonitorUnlock(mon); + qemuMonitorFree(mon); + return 0; + } + + return mon->refs; +} + static int qemuMonitorOpenUnix(const char *monitor) @@ -348,6 +363,7 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) { int quit = 0, failed = 0; qemuMonitorLock(mon); + qemuMonitorRef(mon); VIR_DEBUG("Monitor %p I/O on watch %d fd %d events %d", mon, watch, fd, events); if (mon->fd != fd || mon->watch != watch) { @@ -407,23 +423,17 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) { * but is this safe ? I think it is, because the callback * will try to acquire the virDomainObjPtr mutex next */ if (failed || quit) { + qemuMonitorEOFNotify eofCB = mon->eofCB; + virDomainObjPtr vm = mon->vm; /* Make sure anyone waiting wakes up now */ virCondSignal(&mon->notify); - mon->eofcb = 1; - qemuMonitorUnlock(mon); - VIR_DEBUG("Triggering EOF callback error? %d", failed); - mon->eofCB(mon, mon->vm, failed); - - qemuMonitorLock(mon); - if (mon->closed) { + if (qemuMonitorUnref(mon) > 0) qemuMonitorUnlock(mon); - VIR_DEBUG("Delayed free of monitor %p", mon); - qemuMonitorFree(mon, 1); - } else { - qemuMonitorUnlock(mon); - } + VIR_DEBUG("Triggering EOF callback error? %d", failed); + (eofCB)(mon, vm, failed); } else { - qemuMonitorUnlock(mon); + if (qemuMonitorUnref(mon) > 0) + qemuMonitorUnlock(mon); } } @@ -453,10 +463,10 @@ qemuMonitorOpen(virDomainObjPtr vm, return NULL; } mon->fd = -1; + mon->refs = 1; mon->vm = vm; mon->eofCB = eofCB; qemuMonitorLock(mon); - virDomainObjRef(vm); switch (vm->monitor_chr->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -512,10 +522,14 @@ cleanup: } -void qemuMonitorClose(qemuMonitorPtr mon) +int qemuMonitorClose(qemuMonitorPtr mon) { + int refs; + if (!mon) - return; + return 0; + + VIR_DEBUG("mon=%p", mon); qemuMonitorLock(mon); if (!mon->closed) { @@ -523,18 +537,17 @@ void qemuMonitorClose(qemuMonitorPtr mon) virEventRemoveHandle(mon->watch); if (mon->fd != -1) close(mon->fd); - /* NB: don't reset fd / watch fields, since active - * callback may still want them */ + /* NB: ordinarily one might immediately set mon->watch to -1 + * and mon->fd to -1, but there may be a callback active + * that is still relying on these fields being valid. So + * we merely close them, but not clear their values and + * use this explicit 'closed' flag to track this state */ mon->closed = 1; } - if (mon->eofcb) { - VIR_DEBUG("Mark monitor to be deleted %p", mon); + if ((refs = qemuMonitorUnref(mon)) > 0) qemuMonitorUnlock(mon); - } else { - VIR_DEBUG("Delete monitor now %p", mon); - qemuMonitorFree(mon, 0); - } + return refs; } @@ -552,7 +565,6 @@ int qemuMonitorSend(qemuMonitorPtr mon, if (mon->eofcb) { msg->lastErrno = EIO; - qemuMonitorUnlock(mon); return -1; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 71688cb99..27b43b77f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -78,11 +78,14 @@ typedef int (*qemuMonitorDiskSecretLookup)(qemuMonitorPtr mon, qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, qemuMonitorEOFNotify eofCB); -void qemuMonitorClose(qemuMonitorPtr mon); +int qemuMonitorClose(qemuMonitorPtr mon); void qemuMonitorLock(qemuMonitorPtr mon); void qemuMonitorUnlock(qemuMonitorPtr mon); +int qemuMonitorRef(qemuMonitorPtr mon); +int qemuMonitorUnref(qemuMonitorPtr mon); + void qemuMonitorRegisterDiskSecretLookup(qemuMonitorPtr mon, qemuMonitorDiskSecretLookup secretCB); |