Re: [libvirt] [PATCH 3/6] use virObject to manage reference-count of virDomainObj

On Wed, Apr 06, 2011 at 03:19:51PM +0800, Hu Tao wrote: > This patch also eliminates a dead-lock bug in > qemuDomainObjBeginJobWithDriver: if virCondWaitUntil() timeouts, the > thread tries to acquire qemu driver lock while holding virDomainObj > lock. > --- > src/conf/domain_conf.c | 56 ++++---- > src/conf/domain_conf.h | 6 +- > src/openvz/openvz_conf.c | 8 +- > src/qemu/qemu_domain.c | 32 ++--- > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_driver.c | 304 ++++++++++++++++++++------------------------- > src/qemu/qemu_migration.c | 45 +++---- > src/qemu/qemu_process.c | 33 ++--- > src/vmware/vmware_conf.c | 2 +- > 9 files changed, 215 insertions(+), 273 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 90a1317..fc76a00 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -47,6 +47,7 @@ > #include "storage_file.h" > #include "files.h" > #include "bitmap.h" > +#include "virobject.h" > > #define VIR_FROM_THIS VIR_FROM_DOMAIN > > @@ -395,9 +396,7 @@ static void > virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) > { > virDomainObjPtr obj = payload; > - virDomainObjLock(obj); > - if (virDomainObjUnref(obj) > 0) > - virDomainObjUnlock(obj); > + virDomainObjUnref(obj); > } > > int virDomainObjListInit(virDomainObjListPtr doms) > @@ -437,7 +436,7 @@ virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms, > virDomainObjPtr obj; > obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id); > if (obj) > - virDomainObjLock(obj); > + virDomainObjRef(obj); > return obj; > } > > @@ -452,7 +451,7 @@ virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms, > > obj = virHashLookup(doms->objs, uuidstr); > if (obj) > - virDomainObjLock(obj); > + virDomainObjRef(obj); > return obj; > } > > @@ -476,7 +475,7 @@ virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms, > virDomainObjPtr obj; > obj = virHashSearch(doms->objs, virDomainObjListSearchName, name); > if (obj) > - virDomainObjLock(obj); > + virDomainObjRef(obj); > return obj; > } This is a major change in semantics, which makes pretty much every single caller non-thread safe, unless the callers are all changed todo virDomainObjLock immediately after calling this. So I don't really see the point in this - it just means more code duplication.

> > @@ -967,6 +966,12 @@ static void virDomainObjFree(virDomainObjPtr dom) > { > if (!dom) > return; > + virDomainObjUnref(dom); > +} > + > +static void doDomainObjFree(virObjectPtr obj) > +{ > + virDomainObjPtr dom = (virDomainObjPtr)obj; > > VIR_DEBUG("obj=%p", dom); > virDomainDefFree(dom->def); > @@ -984,21 +989,13 @@ static void virDomainObjFree(virDomainObjPtr dom) > > void virDomainObjRef(virDomainObjPtr dom) > { > - dom->refs++; > - VIR_DEBUG("obj=%p refs=%d", dom, dom->refs); > + virObjectRef(&dom->obj); > } > > > -int virDomainObjUnref(virDomainObjPtr dom) > +void virDomainObjUnref(virDomainObjPtr dom) > { > - dom->refs--; > - VIR_DEBUG("obj=%p refs=%d", dom, dom->refs); > - if (dom->refs == 0) { > - virDomainObjUnlock(dom); > - virDomainObjFree(dom); > - return 0; > - } > - return dom->refs; > + virObjectUnref(&dom->obj); > } > > static virDomainObjPtr virDomainObjNew(virCapsPtr caps) > @@ -1010,6 +1007,11 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps) > return NULL; > } > > + if (virObjectInit(&domain->obj, doDomainObjFree)) { > + VIR_FREE(domain); > + return NULL; > + } > + > if (caps->privateDataAllocFunc && > !(domain->privateData = (caps->privateDataAllocFunc)())) { > virReportOOMError(); > @@ -1027,9 +1029,7 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps) > return NULL; > } > > - virDomainObjLock(domain); > domain->state = VIR_DOMAIN_SHUTOFF; > - domain->refs = 1; > > virDomainSnapshotObjListInit(&domain->snapshots); > > @@ -1075,8 +1075,10 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, > domain->def = def; > > virUUIDFormat(def->uuid, uuidstr); > + virDomainObjRef(domain); > if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) { > - VIR_FREE(domain); > + virDomainObjUnref(domain); > + virDomainObjFree(domain); > return NULL; > } Simiarly here, you're now requiring all callers to manually obtain a lock.

> @@ -1239,39 +1243,57 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, > > qemuDriverLock(driver); > if (!(def = virDomainDefParseString(driver->caps, xml, > - VIR_DOMAIN_XML_INACTIVE))) > + VIR_DOMAIN_XML_INACTIVE))) { > + qemuDriverUnlock(driver); > goto cleanup; > + } > > - if (virSecurityManagerVerify(driver->securityManager, def) < 0) > + if (virSecurityManagerVerify(driver->securityManager, def) < 0) { > + qemuDriverUnlock(driver); > goto cleanup; > + } > > - if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) > + if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) { > + qemuDriverUnlock(driver); > goto cleanup; > + } > > - if (qemudCanonicalizeMachine(driver, def) < 0) > + if (qemudCanonicalizeMachine(driver, def) < 0) { > + qemuDriverUnlock(driver); > goto cleanup; > + } > > - if (qemuDomainAssignPCIAddresses(def) < 0) > + if (qemuDomainAssignPCIAddresses(def) < 0) { > + qemuDriverUnlock(driver); > goto cleanup; > + } > > if (!(vm = virDomainAssignDef(driver->caps, > &driver->domains, > - def, false))) > + def, false))) { > + qemuDriverUnlock(driver); > goto cleanup; > + } > + > + qemuDriverUnlock(driver); driver is now unlocked.... > > def = NULL; > > - if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) > + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) { > + virDomainObjUnref(vm); ...but this method *requires* driver to be locked. > goto cleanup; /* XXXX free the 'vm' we created ? */ > + } > > if (qemuProcessStart(conn, driver, vm, NULL, > (flags & VIR_DOMAIN_START_PAUSED) != 0, > -1, NULL, VIR_VM_OP_CREATE) < 0) { > qemuAuditDomainStart(vm, "booted", false); ...and this method writes to 'driver', so it is now unsafe. > - if (qemuDomainObjEndJob(vm) > 0) > - virDomainRemoveInactive(&driver->domains, > - vm); > - vm = NULL; > + qemuDomainObjEndJob(vm); > + qemuDriverLock(driver); > + virDomainRemoveInactive(&driver->domains, > + vm); > + qemuDriverUnlock(driver); > + virDomainObjUnref(vm); > goto cleanup; > } > > @@ -1283,17 +1305,13 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, > dom = virGetDomain(conn, vm->def->name, vm->def->uuid); > if (dom) dom->id = vm->def->id; > > - if (vm && > - qemuDomainObjEndJob(vm) == 0) > - vm = NULL; > + qemuDomainObjEndJob(vm); > + virDomainObjUnref(vm); > > cleanup: > virDomainDefFree(def); > - if (vm) > - virDomainObjUnlock(vm); > if (event) > qemuDomainEventQueue(driver, event); > - qemuDriverUnlock(driver); > return dom; > } And all the usage of 'vm' in this method is now completely unlocked and unsafe. > > @@ -1307,13 +1325,14 @@ static int qemudDomainSuspend(virDomainPtr dom) { > > qemuDriverLock(driver); > vm = virDomainFindByUUID(&driver->domains, dom->uuid); > + qemuDriverUnlock(driver); > > if (!vm) { > char uuidstr[VIR_UUID_STRING_BUFLEN]; > virUUIDFormat(dom->uuid, uuidstr); > qemuReportError(VIR_ERR_NO_DOMAIN, > _("no domain with matching uuid '%s'"), uuidstr); > - goto cleanup; > + return -1; > } > if (!virDomainObjIsActive(vm)) { > qemuReportError(VIR_ERR_OPERATION_INVALID, > @@ -1354,16 +1373,12 @@ static int qemudDomainSuspend(virDomainPtr dom) { > } > > endjob: > - if (qemuDomainObjEndJob(vm) == 0) > - vm = NULL; > + qemuDomainObjEndJob(vm); > > cleanup: > - if (vm) > - virDomainObjUnlock(vm); > - > + virDomainObjUnref(vm); > if (event) > qemuDomainEventQueue(driver, event); > - qemuDriverUnlock(driver); > return ret; > } Also now completely unsafe since 'vm' is never locked while making changes to it.

[cut rest of patch] I don't see how any of this patch is threadsafe now that virtually no methods are acquiring the 'vm' lock.

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