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

[Daniel P. Berrange]

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.

Regards,
Daniel