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

[Eric Blake]

On 01/24/2011 11:57 PM, 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 agree with the conclusion that unref'ing the monitor too many times
can result in deadlocks.  However, I'm not yet convinced that this patch
is the right solution.

>  
> +    if (mon->closed) {
> +        /* We have closed monitor in other thread. */
> +        qemuMonitorUnlock(mon);
> +        return;
> +    }
> +
> +    mon->closed = true;
> +
>      if (mon->fd >= 0) {

Why can't mon->fd >= 0 be the flag of whether close has previously been
attempted?  After all, once the handle gets removed, VIR_FORCE_CLOSE
sets mon->fd to -1.

>          if (mon->watch)
>              virEventRemoveHandle(mon->watch);

Making qemuMonitorClose() do a short-circuit no-op if we already detect
that another thread has requested close worries me that we might leak
the resource (because we are bypassing the reference counter).  Is there
instead somewhere that we should be temporarily increasing the reference
counter?

This patch may be correct as-is, but I'm not sure of it yet.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110125/0635d027/attachment-0001.sig>