Re: [libvirt] [RFC PATCH 2/2] qemu: use virObject to manage reference-counting for qemu monitor

On Fri, Mar 18, 2011 at 11:20:56AM +0000, Daniel P. Berrange wrote: > On Wed, Mar 16, 2011 at 06:30:20PM +0800, Hu Tao wrote: > > --- > > src/qemu/qemu_domain.c | 26 +----------- > > src/qemu/qemu_monitor.c | 106 ++++++++++++++++++++++++++-------------------- > > src/qemu/qemu_monitor.h | 4 +- > > 3 files changed, 64 insertions(+), 72 deletions(-) > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > index 8a2b9cc..e056ef0 100644 > > --- a/src/qemu/qemu_domain.c > > +++ b/src/qemu/qemu_domain.c > > @@ -566,7 +566,6 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) > > qemuDomainObjPrivatePtr priv = obj->privateData; > > > > qemuMonitorLock(priv->mon); > > - qemuMonitorRef(priv->mon); > > virDomainObjUnlock(obj); > > } > > > > @@ -578,19 +577,9 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) > > void qemuDomainObjExitMonitor(virDomainObjPtr obj) > > { > > qemuDomainObjPrivatePtr priv = obj->privateData; > > - int refs; > > - > > - refs = qemuMonitorUnref(priv->mon); > > - > > - if (refs > 0) > > - qemuMonitorUnlock(priv->mon); > > > > + qemuMonitorUnlock(priv->mon); > > virDomainObjLock(obj); > > - > > - if (refs == 0) { > > - virDomainObjUnref(obj); > > - priv->mon = NULL; > > - } > > } > > > > > > @@ -608,7 +597,6 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, > > qemuDomainObjPrivatePtr priv = obj->privateData; > > > > qemuMonitorLock(priv->mon); > > - qemuMonitorRef(priv->mon); > > virDomainObjUnlock(obj); > > qemuDriverUnlock(driver); > > } > > @@ -623,20 +611,10 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, > > virDomainObjPtr obj) > > { > > qemuDomainObjPrivatePtr priv = obj->privateData; > > - int refs; > > - > > - refs = qemuMonitorUnref(priv->mon); > > - > > - if (refs > 0) > > - qemuMonitorUnlock(priv->mon); > > > > + qemuMonitorUnlock(priv->mon); > > qemuDriverLock(driver); > > virDomainObjLock(obj); > > - > > - if (refs == 0) { > > - virDomainObjUnref(obj); > > - priv->mon = NULL; > > - } > > } > > I don't think these changes aren't right. The EnterMonitor() method > is releasing the lock on the virDomainObjPtr. This means that other > threads (specifically the main I/O thread) can use the virDomainObjPtr > and its qemuMonitorPtr instance. Incrementing reference count on the > qemuMonitorPtr is the only thing that prevents it being free()d while > it is unlocked by this thread. > > eg, consider this > > In the main thread making a monitor call > > qemuDomainObjEnterMonitor(obj) > > qemuMonitorSetCPU(mon, 1, 1); > > qemuDomainObjExitMonitor(obj) > > > So, at step 2 there, qemuMonitorSetCPU() is asleep on the condition > variable, waiting for the reply from QEMU. > > > Now QEMU dies for some reason, so in the I/O thread, we have > qemuMonitorIO() called. This sees the EOF on the socket, so > it invokes the callback "(eofNotify)(mon, vm, failed);". > > This callback is qemuMonitorClose(), which calls qemuMonitorFree() > which releases the last reference count, which deletes the condition > variable. The other thread is now waiting on a condition variable > which has been deleted. It'll either hang forever, or crash > > We *must* prevent the monitor being free'd while a thread is running > a command, hence qemuDomainObjEnterMonitor() must call qemuMonitorRef() > and qemuDomainObjExitMonitor() must call qemuMonitorUnref(). You're all right except on when to ref the monitor. The monitor may be freed by another thread before call of qemuDomainObjEnterMonitor(), so call of qemuMonitorRef() at this time doesn't help. I think the monitor must have already been refed before a thread starts operating it.