Re: [libvirt] [PATCH 2/2] avoid closing monitor twice
At 01/26/2011 12:46 AM, Eric Blake Write:
On 01/24/2011 11:57 PM, Wen Congyang wrote:
> + 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.
We may call qemuMonitorClose() in the function qemuMonitorOpen() to do
the cleanup when it failed. So we still need to close the monitor if
mon->fd is -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?
If we open the monitor successfully, we hold 2 refs: one for open, and another
one for watch. So I think that that we only close monitor once may not leak the
resource.
This patch may be correct as-is, but I'm not sure of it yet.