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

[Daniel P. Berrange]

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.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|