Re: [libvirt] [PATCH v3 05/13] Make virDomainObjList self-locking via virObjectLockable

On Fri, Feb 01, 2013 at 11:18:27 +0000, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > Switch virDomainObjList to inherit from virObjectLockable and > make all the APIs acquire/release the mutex when running. This > makes virDomainObjList completely self-locking and no longer > reliant on the hypervisor driver locks > --- > src/conf/domain_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 61 insertions(+), 14 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 79da5eb..a1e899f 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -60,7 +60,7 @@ verify(VIR_DOMAIN_VIRT_LAST <= 32); > > > struct _virDomainObjList { > - virObject parent; > + virObjectLockable parent; > > /* uuid string -> virDomainObj mapping > * for O(1), lockless lookup-by-uuid */ > @@ -715,7 +715,7 @@ static int virDomainObjOnceInit(void) > virDomainObjDispose))) > return -1; > > - if (!(virDomainObjListClass = virClassNew(virClassForObject(), > + if (!(virDomainObjListClass = virClassNew(virClassForObjectLockable(), > "virDomainObjList", > sizeof(virDomainObjList), > virDomainObjListDispose))) These two hunks should go to 3/13. ... > @@ -1880,25 +1886,32 @@ void virDomainObjAssignDef(virDomainObjPtr domain, > * current def is Live > * > */ > -virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, > - virCapsPtr caps, > - const virDomainDefPtr def, > - unsigned int flags, > - virDomainDefPtr *oldDef) > +static virDomainObjPtr > +virDomainObjListAddLocked(virDomainObjListPtr doms, > + virCapsPtr caps, > + const virDomainDefPtr def, > + unsigned int flags, > + virDomainDefPtr *oldDef) > { > virDomainObjPtr vm; > char uuidstr[VIR_UUID_STRING_BUFLEN]; > + > if (oldDef) > *oldDef = false; > > + virUUIDFormat(def->uuid, uuidstr); > + > /* See if a VM with matching UUID already exists */ > - if ((vm = virDomainObjListFindByUUID(doms, def->uuid))) { > + if ((vm = virHashLookup(doms->objs, uuidstr))) { > + virObjectLock(vm); > /* UUID matches, but if names don't match, refuse it */ > if (STRNEQ(vm->def->name, def->name)) { > virUUIDFormat(vm->def->uuid, uuidstr); > virReportError(VIR_ERR_OPERATION_FAILED, > _("domain '%s' is already defined with uuid %s"), > vm->def->name, uuidstr); > + virObjectUnlock(vm); > + vm = NULL; These two lines should go to 4/13. > goto cleanup; > } > > @@ -1908,6 +1921,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, > virReportError(VIR_ERR_OPERATION_INVALID, > _("domain is already active as '%s'"), > vm->def->name); > + virObjectUnlock(vm); > + vm = NULL; > goto cleanup; > } > } And this hunk should go to 4/13 too. > @@ -1934,12 +1949,11 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, > } > } else { > /* UUID does not match, but if a name matches, refuse it */ > - if ((vm = virDomainObjListFindByName(doms, def->name))) { > + if ((vm = virHashSearch(doms->objs, virDomainObjListSearchName, def->name))) { I believe you wanted to add virObjectLock(vm) here rather than... > virUUIDFormat(vm->def->uuid, uuidstr); > virReportError(VIR_ERR_OPERATION_FAILED, > _("domain '%s' already exists with uuid %s"), > def->name, uuidstr); > - virObjectUnlock(vm); ...removing this unlock here.