We should add an extra reference of vm to avoid vm to be deleted if qemuConnectMonitor() failed.

+++ b/src/qemu/qemu_driver.c @@ -930,6 +930,10 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq priv = obj->privateData; + /* Hold an extra reference because we can't allow 'vm' to be + * deleted if qemuConnectMonitor() failed */ + virDomainObjRef(obj);

The reason of libvirtd cores dump is that: We add vm->refs when we alloc the memory, and decrease it in the function qemuHandleMonitorEOF() in other thread. We add vm->refs in the function qemuConnectMonitor() and decrease it when the vm is inactive. The libvirtd will block in the function qemuMonitorSetCapabilities() because the vm is stopped by signal SIGSTOP. Now the vm->refs is 2. Then we kill the vm by signal SIGKILL. The function qemuMonitorSetCapabilities() failed, and then we will decrease vm->refs in the function qemuMonitorClose(). In another thread, mon->fd is broken and the function qemuHandleMonitorEOF() is called. If qemuHandleMonitorEOF() decreases vm->refs before qemuConnectMonitor() returns, vm->refs will be decrease to 0 and the memory is freed. We will call qemudShutdownVMDaemon() as qemuConnectMonitor() failed. The memory has been freed, so qemudShutdownVMDaemon() is too dangerous. We will reference NULL pointer in the function virDomainConfVMNWFilterTeardown(): ============= void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm) { int i; if (nwfilterDriver != NULL) { for (i = 0; i < vm->def->nnets; i++) virDomainConfNWFilterTeardown(vm->def->nets[i]); } } ============ vm->def->nnets is not 0 but vm->def->nets is NULL(We don't set vm->def->nnets to 0 when we free vm). We should add an extra reference of vm to avoid vm to be deleted if qemuConnectMonitor() failed. Signed-off-by: Wen Congyang <wency(a)cn.fujitsu.com&gt; --- src/qemu/qemu_driver.c | 31 ++++++++++++++++++++++--------- 1 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34cc29f..613c2d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -930,6 +930,10 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq priv = obj->privateData; + /* Hold an extra reference because we can't allow 'vm' to be + * deleted if qemuConnectMonitor() failed */ + virDomainObjRef(obj); + /* XXX check PID liveliness & EXE path */ if (qemuConnectMonitor(driver, obj) < 0) goto error; @@ -961,18 +965,27 @@ qemuReconnectDomain(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaq if (obj->def->id >= driver->nextvmid) driver->nextvmid = obj->def->id + 1; - virDomainObjUnlock(obj); + if (virDomainObjUnref(obj) > 0) + virDomainObjUnlock(obj); return; error: - /* We can't get the monitor back, so must kill the VM - * to remove danger of it ending up running twice if - * user tries to start it again later */ - qemudShutdownVMDaemon(driver, obj, 0); - if (!obj->persistent) - virDomainRemoveInactive(&driver->domains, obj); - else - virDomainObjUnlock(obj); + if (!virDomainObjIsActive(obj)) { + if (virDomainObjUnref(obj) > 0) + virDomainObjUnlock(obj); + return; + } + + if (virDomainObjUnref(obj) > 0) { + /* We can't get the monitor back, so must kill the VM + * to remove danger of it ending up running twice if + * user tries to start it again later */ + qemudShutdownVMDaemon(driver, obj, 0); + if (!obj->persistent) + virDomainRemoveInactive(&driver->domains, obj); + else + virDomainObjUnlock(obj); + } }

On Wed, Jan 26, 2011 at 09:46:52AM +0800, Wen Congyang wrote: > At 01/26/2011 01:02 AM, Daniel P. Berrange Write: > > On Tue, Jan 25, 2011 at 02:57:34PM +0800, Wen Congyang wrote: > >> When we kill the qemu, the function qemuMonitorSetCapabilities() > >> failed and then we close monitor. > >> > >> In another thread, mon->fd is broken and the function > >> qemuHandleMonitorEOF() is called. The function qemuHandleMonitorEOF() calls > >> qemudShutdownVMDaemon() to shutdown vm. The monitor will be > >> closed in the function qemudShutdownVMDaemon(). > >> > >> The monitor close twice and the reference is decreased to 0 unexpectedly. > >> The memory will be freed when reference is decreased to 0. > >> > >> We will remove the watch of mon->fd when the monitor is closed. This > >> request will be done in the function qemuMonitorUnwatch() in the qemuloop > >> thread. In the function qemuMonitorUnwatch(), we will lock monitor, but > >> the lock is destroyed and we will block here, > >> > >> In the main thread, we may add some watch or timeout, and will be blocked > >> because the lock of eventLoop is hold by qemuLoop thread. > >> > >> We should close monitor only once. > > > > I think the problem actually lies in the qemuConnectMonitor() > > call. This method calls qemuMonitorSetCapabilities() and > > if that fails it calls qemuMonitorClose(). > > > > The caller of qemuConnectMonitor() will see the error > > code and also try to kill the QEMU process, by calling > > qemuShutdownVMDaemon(), which calls qemuMonitorClose() > > again. > > > > So I think we need to remove the call to qemuMonitorClose() > > from qemuConnectMonitor() and just let the calls cleanup > > up via normal VM shutdown procedure. > > The function qemudWaitForMonitor() calls qemuConnectMonitor(), > but it does not shutdown VM if qemuConnectMonitor() failed. You need to look further up the call chain. If the call to qemuMonitorSetCapabilities() fails, qemuConnectMOnitor() returns -1. If qemuConnectMonitor() returns -1, then qemuWaitForMonitor() also returns -1. If qemuWaitForMonitor returns -1, then qemudStartVMDaemon will call qemudShutdownVMDaemon which kills the guest and closes the monitor. So, by having qemuConnectMonitor() close the monitor when SetCapabilities() fails, AFAICT, we get a double-close.

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 6140f0f..c527bb7 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -2972,7 +2972,11 @@ cleanup: > * pretend we never started it */ > virCommandFree(cmd); > VIR_FORCE_CLOSE(logfile); > - qemudShutdownVMDaemon(driver, vm, 0); > + /* The vm may be cloesd in other thread, so we should check whether the s/cloesd/closed/ > + * vm is active before shutdown. > + */ > + if (virDomainObjIsActive(vm)) > + qemudShutdownVMDaemon(driver, vm, 0); I'm still playing with this patch, but at first glance, it is making sense to me.

On Wed, Feb 02, 2011 at 11:37:37 -0700, Eric Blake wrote: > * src/qemu/qemu_driver.c (qemudShutdownVMDaemon): Check that vm is > still active. > Reported by Wen Congyang as follows: > > --- > +++ b/src/qemu/qemu_driver.c > @@ -2991,6 +2991,11 @@ static void qemudShutdownVMDaemon(struct qemud_driver *driver, > VIR_DEBUG("Shutting down VM '%s' pid=%d migrated=%d", > vm->def->name, vm->pid, migrated); > > + if (!virDomainObjIsActive(vm)) { > + VIR_DEBUG("VM '%s' not active", vm->def->name); > + return; > + } > + Looks like the solution Dan was asking for... ACK