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

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; }

@@ -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; }

@@ -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);

def = NULL; - if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) { + virDomainObjUnref(vm);

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);

- 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; }

@@ -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; }