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?
> +++ 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list