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

Eric Blake eblake at redhat.com
Tue Jan 15 23:25:54 UTC 2013


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?

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130115/c7ee0dbc/attachment-0001.sig>


More information about the libvir-list mailing list