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@xxxxxxxxxx>
> >
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list