[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



At 01/26/2011 06:49 PM, Daniel P. Berrange Write:
> 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.

Yes, you are right.
Another question:
I think we should check whether the vm has been closed before calling
qemudShutdownVMDaemon() in the function qemudStartVMDaemon() as the vm
may be shutdown in the function qemuHandleMonitorEOF() in another thread.


> 
> So, by having qemuConnectMonitor() close the monitor when
> SetCapabilities() fails, AFAICT, we get a double-close.
> 
> Regards,
> Daniel
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]