[libvirt] [PATCH 3/7] Convert virDomainObj, qemuAgent, qemuMonitor, lxcMonitor to virObjectLockable

[Daniel P. Berrange]

On Tue, Jan 15, 2013 at 04:25:54PM -0700, Eric Blake wrote:
> On 01/11/2013 05:13 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > The  virDomainObj, qemuAgent, qemuMonitor, lxcMonitor classes
> > all require a mutex, so can be switched to use virObjectLockable
> > ---
> 
> >  27 files changed, 481 insertions(+), 579 deletions(-)
> 
> Big, but looks mostly mechanical.
> 
> > +++ b/src/conf/domain_conf.h
> > @@ -1858,9 +1858,7 @@ struct _virDomainStateReason {
> >  typedef struct _virDomainObj virDomainObj;
> >  typedef virDomainObj *virDomainObjPtr;
> >  struct _virDomainObj {
> > -    virObject object;
> > -
> > -    virMutex lock;
> > +    virObjectLockable parent;
> 
> A few of these hunks form the real meat of the change, with everything
> else being fallout of using the benefits of the new parent class.
> 
> > @@ -733,26 +720,18 @@ qemuAgentOpen(virDomainObjPtr vm,
> >      if (qemuAgentInitialize() < 0)
> >          return NULL;
> >  
> > -    if (!(mon = virObjectNew(qemuAgentClass)))
> > +    if (!(mon = virObjectLockableNew(qemuAgentClass)))
> >          return NULL;
> >  
> > -    if (virMutexInit(&mon->lock) < 0) {
> > -        virReportSystemError(errno, "%s",
> > -                             _("cannot initialize monitor mutex"));
> > -        VIR_FREE(mon);
> > -        return NULL;
> > -    }
> > +    mon->fd = -1;
> 
> Yep, I can see why you had to hoist this...
> 
> >      if (virCondInit(&mon->notify) < 0) {
> >          virReportSystemError(errno, "%s",
> >                               _("cannot initialize monitor condition"));
> > -        virMutexDestroy(&mon->lock);
> > -        VIR_FREE(mon);
> > +        virObjectUnref(mon);
> >          return NULL;
> 
> ...when you replaced ad hoc cleanup by the parent class cleanup.
> 
> >      }
> > -    mon->fd = -1;
> >      mon->vm = vm;
> >      mon->cb = cb;
> > -    qemuAgentLock(mon);
> >  
> >      switch (config->type) {
> >      case VIR_DOMAIN_CHR_TYPE_UNIX:
> > @@ -791,7 +770,6 @@ qemuAgentOpen(virDomainObjPtr vm,
> >      virObjectRef(mon);
> >  
> >      VIR_DEBUG("New mon %p fd =%d watch=%d", mon, mon->fd, mon->watch);
> > -    qemuAgentUnlock(mon);
> 
> Question - was it really safe to remove the lock around this section of
> code, considering that you were previously handing 'mon' to
> virEventAddHandle() only inside the lock?  That is, now that locks are
> dropped, is there any possibility that the handle you just added could
> fire in the window between virEventAddHandle() and virObjectRef(), such
> that you end up calling virObjectFreeCallback too soon and we end up
> calling virObjectRef on a stale object?

I believe it is safe, but for sanity I'll move the ref. Also the same
code in QEMU monitor.

> > +++ b/src/qemu/qemu_domain.c
> > @@ -786,12 +786,12 @@ retry:
> >      }
> >  
> >      while (!nested && !qemuDomainNestedJobAllowed(priv, job)) {
> > -        if (virCondWaitUntil(&priv->job.asyncCond, &obj->lock, then) < 0)
> > +        if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0)
> 
> Feels a bit weird accessing the parent lock in this manner; maybe a
> virObjectGetLock(&obj) accessor would have been easier to read.  But I'm
> not too concerned; this works as-is.

Might be worth adding a virObjectLockableWait() call which accepts
a virCondPtr arg. Can do this as a followup

> Assuming the dropped qemuAgentLock() was safe,
> 
> ACK.

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 :|