[libvirt] [PATCH 4/6] qemu: use virObject to manage reference-counting for qemu monitor

[Hu Tao]

On Fri, Apr 08, 2011 at 09:57:16AM +0100, Daniel P. Berrange wrote:
> On Fri, Apr 08, 2011 at 11:07:44AM +0800, Hu Tao wrote:
> > On Thu, Apr 07, 2011 at 10:38:34AM +0100, Daniel P. Berrange wrote:
> > > On Wed, Apr 06, 2011 at 03:19:55PM +0800, Hu Tao wrote:
> > > > ---
> > > >  src/qemu/qemu_domain.c    |   30 ++-----------
> > > >  src/qemu/qemu_migration.c |    2 -
> > > >  src/qemu/qemu_monitor.c   |  109 ++++++++++++++++++++++++--------------------
> > > >  src/qemu/qemu_monitor.h   |    4 +-
> > > >  src/qemu/qemu_process.c   |   32 +++++++-------
> > > >  5 files changed, 81 insertions(+), 96 deletions(-)
> > > > 
> > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > > index 3a3c953..d11dc5f 100644
> > > > --- a/src/qemu/qemu_domain.c
> > > > +++ b/src/qemu/qemu_domain.c
> > > > @@ -560,9 +560,8 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
> > > >  {
> > > >      qemuDomainObjPrivatePtr priv = obj->privateData;
> > > >  
> > > > -    qemuMonitorLock(priv->mon);
> > > > -    qemuMonitorRef(priv->mon);
> > > >      virDomainObjUnlock(obj);
> > > > +    qemuMonitorLock(priv->mon);
> > > >  }
> > > >  
> > > >  
> > > > @@ -573,18 +572,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) {
> > > > -        priv->mon = NULL;
> > > > -    }
> > > >  }
> > > >  
> > > >  
> > > > @@ -601,10 +591,8 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver,
> > > >  {
> > > >      qemuDomainObjPrivatePtr priv = obj->privateData;
> > > >  
> > > > -    qemuMonitorLock(priv->mon);
> > > > -    qemuMonitorRef(priv->mon);
> > > >      virDomainObjUnlock(obj);
> > > > -    qemuDriverUnlock(driver);
> > > > +    qemuMonitorLock(priv->mon);
> > > >  }
> > > >  
> > > >  
> > > > @@ -617,19 +605,9 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver,
> > > >                                          virDomainObjPtr obj)
> > > >  {
> > > >      qemuDomainObjPrivatePtr priv = obj->privateData;
> > > > -    int refs;
> > > > -
> > > > -    refs = qemuMonitorUnref(priv->mon);
> > > > -
> > > > -    if (refs > 0)
> > > > -        qemuMonitorUnlock(priv->mon);
> > > >  
> > > > -    qemuDriverLock(driver);
> > > > +    qemuMonitorUnlock(priv->mon);
> > > >      virDomainObjLock(obj);
> > > > -
> > > > -    if (refs == 0) {
> > > > -        priv->mon = NULL;
> > > > -    }
> > > >  }
> > > 
> > > This means that the 'driver' lock is now whenever any QEMU
> > > monitor command is runing, which blocks the entire driver.
> > 
> > qemuDomainObjEnterMonitorWithDriver is now called without holding qemu
> > driver lock.
> 
> Well, this and the other changes in this series are completely
> altering all the locking rules used throughout the QEMU
> driver, with no clear explanation of what you are actually
> doing. Please read src/qemu/THREADS.txt and then provide an
> equivalent document explaining what you think the new rules
> should be, otherwise it is impossible to tell if these patches
> are at all threadsafe.

Sorry I didn't make this clear, will do in next version.