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

On 01/11/2013 05:13 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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?

> +++ 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.

Assuming the dropped qemuAgentLock() was safe, ACK.