[libvirt] [PATCH 2/2] avoid closing monitor twice

[Daniel P. Berrange]

On Wed, Jan 26, 2011 at 10:49:28AM +0000, Daniel P. Berrange wrote:
> 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.

I propose this patch, as an alternative to your patch 2/2:

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b6a5cd6..5050921 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -900,8 +900,6 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm)
     qemuDomainObjExitMonitorWithDriver(driver, vm);
 
 error:
-    if (ret < 0)
-        qemuMonitorClose(priv->mon);
 
     return ret;
 }


In combination with your patch 1/2, this fixes the double-free
in reconnecting to QEMU and passes valgrind's checks


Daniel