[libvirt] [PATCH 0/8] Test driver refactors and fixes
Note that patch 1/8 of this series depends on patch 2/13 of the "vCPU pinning and related refactors - Part 1.5" series: http://www.redhat.com/archives/libvir-list/2015-June/msg00678.html Peter Krempa (8): test: Reuse virDomainObjGetOneDef in testDomainGetVcpusFlags test: Switch to reference counting with testDomObjFromDomain test: Refactor test driver domain object retrieval test: Refactor test driver event sending test: group domain APIs together lib: setvcpus: Remove bogous flag check test: Refactor testDomainSetVcpusFlags test: Fix lock ordering in testDomainRevertToSnapshot src/libvirt-domain.c | 6 - src/test/test_driver.c | 941 +++++++++++++++++-------------------------------- 2 files changed, 324 insertions(+), 623 deletions(-) -- 2.4.1
The test driver copies the domain definition correctly so we can reuse the helper. --- src/test/test_driver.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d1f0af3..c0ef459 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2576,13 +2576,9 @@ testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) goto cleanup; } - if (virDomainLiveConfigHelperMethod(privconn->caps, privconn->xmlopt, - vm, &flags, &def) < 0) + if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; - if (flags & VIR_DOMAIN_AFFECT_LIVE) - def = vm->def; - ret = (flags & VIR_DOMAIN_VCPU_MAXIMUM) ? def->maxvcpus : def->vcpus; cleanup: -- 2.4.1
Retrieve domain objects with reference and release them with virDomainObjEndAPI. --- src/test/test_driver.c | 89 ++++++++++++++++++++------------------------------ 1 file changed, 35 insertions(+), 54 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c0ef459..6613ed7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -484,7 +484,7 @@ testDomObjFromDomain(virDomainPtr domain) char uuidstr[VIR_UUID_STRING_BUFLEN]; testDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); + vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -6262,19 +6262,17 @@ static int testDomainSnapshotNum(virDomainPtr domain, unsigned int flags) { virDomainObjPtr vm = NULL; - int n = -1; + int n; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromDomain(domain))) - goto cleanup; + return -1; n = virDomainSnapshotObjListNum(vm->snapshots, NULL, flags); - cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return n; } @@ -6285,20 +6283,18 @@ testDomainSnapshotListNames(virDomainPtr domain, unsigned int flags) { virDomainObjPtr vm = NULL; - int n = -1; + int n; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromDomain(domain))) - goto cleanup; + return -1; n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen, flags); - cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return n; } @@ -6308,19 +6304,17 @@ testDomainListAllSnapshots(virDomainPtr domain, unsigned int flags) { virDomainObjPtr vm = NULL; - int n = -1; + int n; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromDomain(domain))) - goto cleanup; + return -1; n = virDomainListSnapshots(vm->snapshots, NULL, domain, snaps, flags); - cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return n; } @@ -6338,7 +6332,7 @@ testDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) goto cleanup; @@ -6347,8 +6341,7 @@ testDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return n; } @@ -6364,7 +6357,7 @@ testDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) goto cleanup; @@ -6372,8 +6365,7 @@ testDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, n = virDomainSnapshotObjListNum(vm->snapshots, snap, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return n; } @@ -6390,7 +6382,7 @@ testDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) goto cleanup; @@ -6399,8 +6391,7 @@ testDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return n; } @@ -6416,7 +6407,7 @@ testDomainSnapshotLookupByName(virDomainPtr domain, virCheckFlags(0, NULL); if (!(vm = testDomObjFromDomain(domain))) - goto cleanup; + return NULL; if (!(snap = testSnapObjFromName(vm, name))) goto cleanup; @@ -6424,8 +6415,7 @@ testDomainSnapshotLookupByName(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return snapshot; } @@ -6434,18 +6424,16 @@ testDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags) { virDomainObjPtr vm; - int ret = -1; + int ret; virCheckFlags(0, -1); if (!(vm = testDomObjFromDomain(domain))) - goto cleanup; + return -1; ret = (vm->current_snapshot != NULL); - cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -6460,7 +6448,7 @@ testDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, virCheckFlags(0, NULL); if (!(vm = testDomObjFromSnapshot(snapshot))) - goto cleanup; + return NULL; if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) goto cleanup; @@ -6475,8 +6463,7 @@ testDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, parent = virGetDomainSnapshot(snapshot->domain, snap->def->parent); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return parent; } @@ -6490,7 +6477,7 @@ testDomainSnapshotCurrent(virDomainPtr domain, virCheckFlags(0, NULL); if (!(vm = testDomObjFromDomain(domain))) - goto cleanup; + return NULL; if (!vm->current_snapshot) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, "%s", @@ -6501,8 +6488,7 @@ testDomainSnapshotCurrent(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, vm->current_snapshot->def->name); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return snapshot; } @@ -6518,7 +6504,7 @@ testDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); if (!(vm = testDomObjFromSnapshot(snapshot))) - goto cleanup; + return NULL; if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) goto cleanup; @@ -6530,8 +6516,7 @@ testDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, 0); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return xml; } @@ -6540,19 +6525,17 @@ testDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, unsigned int flags) { virDomainObjPtr vm = NULL; - int ret = -1; + int ret; virCheckFlags(0, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; ret = (vm->current_snapshot && STREQ(snapshot->name, vm->current_snapshot->def->name)); - cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -6567,7 +6550,7 @@ testDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, virCheckFlags(0, -1); if (!(vm = testDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (!testSnapObjFromSnapshot(vm, snapshot)) goto cleanup; @@ -6575,8 +6558,7 @@ testDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, ret = 1; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -6717,7 +6699,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, snap->sibling = other->first_child; other->first_child = snap; } - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); } if (event) { testDriverLock(privconn); @@ -6854,8 +6836,7 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -7067,7 +7048,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } else { virObjectUnref(event2); } - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); testDriverUnlock(privconn); return ret; -- 2.4.1
On 16.06.2015 19:43, Peter Krempa wrote:
Retrieve domain objects with reference and release them with virDomainObjEndAPI. --- src/test/test_driver.c | 89 ++++++++++++++++++++------------------------------ 1 file changed, 35 insertions(+), 54 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c0ef459..6613ed7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -484,7 +484,7 @@ testDomObjFromDomain(virDomainPtr domain) char uuidstr[VIR_UUID_STRING_BUFLEN];
testDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); + vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN,
Oh, this reminds me that I wanted to drop testDriverLock. It's unnecessary here - domainObjList has self-locking APIs. ACK Michal
Add testDomObjFromDomainLocked and reuse it together with testDomObjFromDomain to retrieve domain objects in the qemu driver instead of open-coding it in every API. --- src/test/test_driver.c | 409 +++++++++++-------------------------------------- 1 file changed, 92 insertions(+), 317 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6613ed7..dc6e49a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -477,13 +477,12 @@ static int testStoragePoolObjSetDefaults(virStoragePoolObjPtr pool); static int testNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info); static virDomainObjPtr -testDomObjFromDomain(virDomainPtr domain) +testDomObjFromDomainLocked(testConnPtr driver, + virDomainPtr domain) { virDomainObjPtr vm; - testConnPtr driver = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - testDriverLock(driver); vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); @@ -492,10 +491,22 @@ testDomObjFromDomain(virDomainPtr domain) uuidstr, domain->name); } - testDriverUnlock(driver); return vm; } +static virDomainObjPtr +testDomObjFromDomain(virDomainPtr domain) +{ + testConnPtr driver = domain->conn->privateData; + virDomainObjPtr ret; + + testDriverLock(driver); + ret = testDomObjFromDomainLocked(driver, domain); + testDriverUnlock(driver); + + return ret; +} + static char * testDomainGenerateIfname(virDomainDefPtr domdef) { @@ -1688,43 +1699,28 @@ static int testConnectNumOfDomains(virConnectPtr conn) static int testDomainIsActive(virDomainPtr dom) { - testConnPtr privconn = dom->conn->privateData; virDomainObjPtr obj; - int ret = -1; + int ret; - testDriverLock(privconn); - obj = virDomainObjListFindByUUID(privconn->domains, dom->uuid); - testDriverUnlock(privconn); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } - ret = virDomainObjIsActive(obj); + if (!(obj = testDomObjFromDomain(dom))) + return -1; - cleanup: - if (obj) - virObjectUnlock(obj); + ret = virDomainObjIsActive(obj); + virDomainObjEndAPI(&obj); return ret; } static int testDomainIsPersistent(virDomainPtr dom) { - testConnPtr privconn = dom->conn->privateData; virDomainObjPtr obj; - int ret = -1; + int ret; + + if (!(obj = testDomObjFromDomain(dom))) + return -1; - testDriverLock(privconn); - obj = virDomainObjListFindByUUID(privconn->domains, dom->uuid); - testDriverUnlock(privconn); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } ret = obj->persistent; - cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -1885,13 +1881,9 @@ static int testDomainDestroy(virDomainPtr domain) int ret = -1; testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(privdom = testDomObjFromDomainLocked(privconn, domain))) goto cleanup; - } testDomainShutdownState(domain, privdom, VIR_DOMAIN_SHUTOFF_DESTROYED); event = virDomainEventLifecycleNewFromObj(privdom, @@ -1917,15 +1909,8 @@ static int testDomainResume(virDomainPtr domain) virObjectEventPtr event = NULL; int ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; if (virDomainObjGetState(privdom, NULL) != VIR_DOMAIN_PAUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, _("domain '%s' not paused"), @@ -1958,15 +1943,8 @@ static int testDomainSuspend(virDomainPtr domain) int ret = -1; int state; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; state = virDomainObjGetState(privdom, NULL); if (state == VIR_DOMAIN_SHUTOFF || state == VIR_DOMAIN_PAUSED) { @@ -2003,13 +1981,9 @@ static int testDomainShutdownFlags(virDomainPtr domain, virCheckFlags(0, -1); testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(privdom = testDomObjFromDomainLocked(privconn, domain))) goto cleanup; - } if (virDomainObjGetState(privdom, NULL) == VIR_DOMAIN_SHUTOFF) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2049,13 +2023,9 @@ static int testDomainReboot(virDomainPtr domain, int ret = -1; testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(privdom = testDomObjFromDomainLocked(privconn, domain))) goto cleanup; - } virDomainObjSetState(privdom, VIR_DOMAIN_SHUTDOWN, VIR_DOMAIN_SHUTDOWN_USER); @@ -2109,20 +2079,12 @@ static int testDomainReboot(virDomainPtr domain, static int testDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) { - testConnPtr privconn = domain->conn->privateData; struct timeval tv; virDomainObjPtr privdom; int ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; if (gettimeofday(&tv, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2148,28 +2110,18 @@ testDomainGetState(virDomainPtr domain, int *reason, unsigned int flags) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; - int ret = -1; virCheckFlags(0, -1); - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; *state = virDomainObjGetState(privdom, reason); - ret = 0; - cleanup: virDomainObjEndAPI(&privdom); - return ret; + + return 0; } #define TEST_SAVE_MAGIC "TestGuestMagic" @@ -2194,13 +2146,9 @@ testDomainSaveFlags(virDomainPtr domain, const char *path, } testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(privdom = testDomObjFromDomainLocked(privconn, domain))) goto cleanup; - } xml = virDomainDefFormat(privdom->def, VIR_DOMAIN_DEF_FORMAT_SECURE); @@ -2398,13 +2346,9 @@ static int testDomainCoreDumpWithFormat(virDomainPtr domain, virCheckFlags(VIR_DUMP_CRASH, -1); testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(privdom = testDomObjFromDomainLocked(privconn, domain))) goto cleanup; - } if ((fd = open(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { virReportSystemError(errno, @@ -2475,23 +2419,14 @@ testDomainGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED) static unsigned long long testDomainGetMaxMemory(virDomainPtr domain) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; unsigned long long ret = 0; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return 0; ret = virDomainDefGetMemoryActual(privdom->def); - cleanup: virDomainObjEndAPI(&privdom); return ret; } @@ -2499,45 +2434,26 @@ testDomainGetMaxMemory(virDomainPtr domain) static int testDomainSetMaxMemory(virDomainPtr domain, unsigned long memory) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; - int ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; /* XXX validate not over host memory wrt to other domains */ virDomainDefSetMemoryInitial(privdom->def, memory); - ret = 0; - cleanup: virDomainObjEndAPI(&privdom); - return ret; + return 0; } static int testDomainSetMemory(virDomainPtr domain, unsigned long memory) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; int ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; if (memory > virDomainDefGetMemoryActual(privdom->def)) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -2555,7 +2471,6 @@ static int testDomainSetMemory(virDomainPtr domain, static int testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr vm; virDomainDefPtr def; int ret = -1; @@ -2564,17 +2479,8 @@ testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - testDriverLock(privconn); - vm = virDomainObjListFindByUUID(privconn->domains, domain->uuid); - testDriverUnlock(privconn); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(domain->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = testDomObjFromDomain(domain))) + return -1; if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; @@ -2582,8 +2488,7 @@ testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) ret = (flags & VIR_DOMAIN_VCPU_MAXIMUM) ? def->maxvcpus : def->vcpus; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2622,14 +2527,8 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, return -1; } - testDriverLock(privconn); - privdom = virDomainObjListFindByUUID(privconn->domains, domain->uuid); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; if (!virDomainObjIsActive(privdom) && (flags & VIR_DOMAIN_AFFECT_LIVE)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2680,8 +2579,7 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, } cleanup: - if (privdom) - virObjectUnlock(privdom); + virDomainObjEndAPI(&privdom); return ret; } @@ -2706,14 +2604,8 @@ static int testDomainGetVcpus(virDomainPtr domain, struct timeval tv; unsigned long long statbase; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; if (!virDomainObjIsActive(privdom)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2791,14 +2683,8 @@ static int testDomainPinVcpu(virDomainPtr domain, int maxcpu, hostcpus, privmaplen; int ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; if (!virDomainObjIsActive(privdom)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2836,30 +2722,20 @@ static int testDomainPinVcpu(virDomainPtr domain, static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) { - testConnPtr privconn = domain->conn->privateData; virDomainDefPtr def; virDomainObjPtr privdom; char *ret = NULL; /* Flags checked by virDomainDefFormat */ - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return NULL; def = (flags & VIR_DOMAIN_XML_INACTIVE) && privdom->newDef ? privdom->newDef : privdom->def; - ret = virDomainDefFormat(def, - virDomainDefFormatConvertXMLFlags(flags)); + ret = virDomainDefFormat(def, virDomainDefFormatConvertXMLFlags(flags)); - cleanup: virDomainObjEndAPI(&privdom); return ret; } @@ -2960,25 +2836,17 @@ static char *testDomainGetMetadata(virDomainPtr dom, { testConnPtr privconn = dom->conn->privateData; virDomainObjPtr privdom; - char *ret = NULL; + char *ret; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, NULL); - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - dom->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(dom))) + return NULL; ret = virDomainObjGetMetadata(privdom, type, uri, privconn->caps, privconn->xmlopt, flags); - cleanup: virDomainObjEndAPI(&privdom); return ret; } @@ -2992,26 +2860,18 @@ static int testDomainSetMetadata(virDomainPtr dom, { testConnPtr privconn = dom->conn->privateData; virDomainObjPtr privdom; - int ret = -1; + int ret; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - dom->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(dom))) + return -1; ret = virDomainObjSetMetadata(privdom, type, metadata, key, uri, privconn->caps, privconn->xmlopt, NULL, NULL, flags); - cleanup: virDomainObjEndAPI(&privdom); return ret; } @@ -3056,13 +2916,9 @@ static int testDomainCreateWithFlags(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(privdom = testDomObjFromDomainLocked(privconn, domain))) goto cleanup; - } if (virDomainObjGetState(privdom, NULL) != VIR_DOMAIN_SHUTOFF) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3106,13 +2962,9 @@ static int testDomainUndefineFlags(virDomainPtr domain, VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); + if (!(privdom = testDomObjFromDomainLocked(privconn, domain))) goto cleanup; - } if (privdom->hasManagedSave && !(flags & VIR_DOMAIN_UNDEFINE_MANAGED_SAVE)) { @@ -3168,52 +3020,30 @@ static int testDomainUndefine(virDomainPtr domain) static int testDomainGetAutostart(virDomainPtr domain, int *autostart) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; - int ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; *autostart = privdom->autostart; - ret = 0; - cleanup: virDomainObjEndAPI(&privdom); - return ret; + return 0; } static int testDomainSetAutostart(virDomainPtr domain, int autostart) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; - int ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; privdom->autostart = autostart ? 1 : 0; - ret = 0; - cleanup: virDomainObjEndAPI(&privdom); - return ret; + return 0; } static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, @@ -3235,21 +3065,13 @@ testDomainGetSchedulerParametersFlags(virDomainPtr domain, int *nparams, unsigned int flags) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; int ret = -1; virCheckFlags(0, -1); - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; if (virTypedParameterAssign(params, VIR_DOMAIN_SCHEDULER_WEIGHT, VIR_TYPED_PARAM_UINT, 50) < 0) @@ -3279,7 +3101,6 @@ testDomainSetSchedulerParametersFlags(virDomainPtr domain, int nparams, unsigned int flags) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; int ret = -1; size_t i; @@ -3291,15 +3112,8 @@ testDomainSetSchedulerParametersFlags(virDomainPtr domain, NULL) < 0) return -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; for (i = 0; i < nparams; i++) { if (STREQ(params[i].field, VIR_DOMAIN_SCHEDULER_WEIGHT)) { @@ -3310,7 +3124,6 @@ testDomainSetSchedulerParametersFlags(virDomainPtr domain, ret = 0; - cleanup: virDomainObjEndAPI(&privdom); return ret; } @@ -3327,7 +3140,6 @@ static int testDomainBlockStats(virDomainPtr domain, const char *path, virDomainBlockStatsPtr stats) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; struct timeval tv; unsigned long long statbase; @@ -3339,15 +3151,8 @@ static int testDomainBlockStats(virDomainPtr domain, return ret; } - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + if (!(privdom = testDomObjFromDomain(domain))) + return ret; if (virDomainDiskIndexByName(privdom->def, path, false) < 0) { virReportError(VIR_ERR_INVALID_ARG, @@ -3379,22 +3184,14 @@ static int testDomainInterfaceStats(virDomainPtr domain, const char *path, virDomainInterfaceStatsPtr stats) { - testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; struct timeval tv; unsigned long long statbase; size_t i; int found = 0, ret = -1; - testDriverLock(privconn); - privdom = virDomainObjListFindByName(privconn->domains, - domain->name); - testDriverUnlock(privconn); - - if (privdom == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto error; - } + if (!(privdom = testDomObjFromDomain(domain))) + return -1; for (i = 0; i < privdom->def->nnets; i++) { if (privdom->def->nets[i]->ifname && @@ -6138,14 +5935,8 @@ testDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED, -1); - testDriverLock(privconn); - vm = virDomainObjListFindByName(privconn->domains, dom->name); - testDriverUnlock(privconn); - - if (vm == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(vm = testDomObjFromDomain(dom))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -6181,50 +5972,34 @@ testDomainManagedSave(virDomainPtr dom, unsigned int flags) static int testDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) { - testConnPtr privconn = dom->conn->privateData; virDomainObjPtr vm; - int ret = -1; + int ret; virCheckFlags(0, -1); - testDriverLock(privconn); - - vm = virDomainObjListFindByName(privconn->domains, dom->name); - if (vm == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(vm = testDomObjFromDomain(dom))) + return -1; ret = vm->hasManagedSave; - cleanup: + virDomainObjEndAPI(&vm); - testDriverUnlock(privconn); return ret; } static int testDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) { - testConnPtr privconn = dom->conn->privateData; virDomainObjPtr vm; - int ret = -1; virCheckFlags(0, -1); - testDriverLock(privconn); - - vm = virDomainObjListFindByName(privconn->domains, dom->name); - if (vm == NULL) { - virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); - goto cleanup; - } + if (!(vm = testDomObjFromDomain(dom))) + return -1; vm->hasManagedSave = false; - ret = 0; - cleanup: + virDomainObjEndAPI(&vm); - testDriverUnlock(privconn); - return ret; + return 0; } -- 2.4.1
On 16.06.2015 19:43, Peter Krempa wrote:
Add testDomObjFromDomainLocked and reuse it together with testDomObjFromDomain to retrieve domain objects in the qemu driver
s/qemu/test/
instead of open-coding it in every API. --- src/test/test_driver.c | 409 +++++++++++-------------------------------------- 1 file changed, 92 insertions(+), 317 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6613ed7..dc6e49a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -477,13 +477,12 @@ static int testStoragePoolObjSetDefaults(virStoragePoolObjPtr pool); static int testNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
static virDomainObjPtr -testDomObjFromDomain(virDomainPtr domain) +testDomObjFromDomainLocked(testConnPtr driver, + virDomainPtr domain) { virDomainObjPtr vm; - testConnPtr driver = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN];
- testDriverLock(driver); vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); @@ -492,10 +491,22 @@ testDomObjFromDomain(virDomainPtr domain) uuidstr, domain->name); }
- testDriverUnlock(driver); return vm; }
+static virDomainObjPtr +testDomObjFromDomain(virDomainPtr domain) +{ + testConnPtr driver = domain->conn->privateData; + virDomainObjPtr ret; + + testDriverLock(driver); + ret = testDomObjFromDomainLocked(driver, domain); + testDriverUnlock(driver); + + return ret; +} +
I don't think this is necessary. As I've said in the previous e-mail - domainObjList has self-locking APIs. And the driver->domains is immutable pointer. Or am I missing something? ACK to the rest though. Michal
Make testObjectEventQueue tolerant to NULL @event and move it so that it does not require a prototype. Additionally add testObjectEventQueueUnlocked that will lock @driver before sending the event. Refactor the rest of the codebase to make use of the above features --- src/test/test_driver.c | 112 +++++++++++++++++++------------------------------ 1 file changed, 44 insertions(+), 68 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index dc6e49a..20d3c71 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -137,8 +137,6 @@ static const virNodeInfo defaultNodeInfo = { static int testConnectClose(virConnectPtr conn); -static void testObjectEventQueue(testConnPtr driver, - virObjectEventPtr event); static void testDriverLock(testConnPtr driver) { @@ -150,6 +148,28 @@ static void testDriverUnlock(testConnPtr driver) virMutexUnlock(&driver->lock); } +static void testObjectEventQueue(testConnPtr driver, + virObjectEventPtr event) +{ + if (!event) + return; + + virObjectEventStateQueue(driver->eventState, event); +} + + +static void +testObjectEventQueueUnlocked(testConnPtr driver, + virObjectEventPtr event) +{ + if (!event) + return; + + testDriverLock(driver); + testObjectEventQueue(driver, event); + testDriverUnlock(driver); +} + static void *testDomainObjPrivateAlloc(void) { testDomainObjPrivatePtr priv; @@ -1774,8 +1794,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, cleanup: if (dom) virObjectUnlock(dom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); virDomainDefFree(def); testDriverUnlock(privconn); return ret; @@ -1896,8 +1915,7 @@ static int testDomainDestroy(virDomainPtr domain) ret = 0; cleanup: virDomainObjEndAPI(&privdom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -1927,11 +1945,7 @@ static int testDomainResume(virDomainPtr domain) cleanup: virDomainObjEndAPI(&privdom); - if (event) { - testDriverLock(privconn); - testObjectEventQueue(privconn, event); - testDriverUnlock(privconn); - } + testObjectEventQueueUnlocked(privconn, event); return ret; } @@ -1961,12 +1975,7 @@ static int testDomainSuspend(virDomainPtr domain) cleanup: virDomainObjEndAPI(&privdom); - - if (event) { - testDriverLock(privconn); - testObjectEventQueue(privconn, event); - testDriverUnlock(privconn); - } + testObjectEventQueueUnlocked(privconn, event); return ret; } @@ -2002,8 +2011,7 @@ static int testDomainShutdownFlags(virDomainPtr domain, ret = 0; cleanup: virDomainObjEndAPI(&privdom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -2070,8 +2078,7 @@ static int testDomainReboot(virDomainPtr domain, ret = 0; cleanup: virDomainObjEndAPI(&privdom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -2214,8 +2221,7 @@ testDomainSaveFlags(virDomainPtr domain, const char *path, unlink(path); } virDomainObjEndAPI(&privdom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -2319,8 +2325,7 @@ testDomainRestoreFlags(virConnectPtr conn, VIR_FORCE_CLOSE(fd); if (dom) virObjectUnlock(dom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -2389,8 +2394,7 @@ static int testDomainCoreDumpWithFormat(virDomainPtr domain, cleanup: VIR_FORCE_CLOSE(fd); virDomainObjEndAPI(&privdom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -2817,8 +2821,7 @@ static virDomainPtr testDomainDefineXMLFlags(virConnectPtr conn, virDomainDefFree(oldDef); if (dom) virObjectUnlock(dom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -2938,8 +2941,7 @@ static int testDomainCreateWithFlags(virDomainPtr domain, unsigned int flags) cleanup: virDomainObjEndAPI(&privdom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -3006,8 +3008,7 @@ static int testDomainUndefineFlags(virDomainPtr domain, cleanup: virDomainObjEndAPI(&privdom); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); testDriverUnlock(privconn); return ret; } @@ -3385,8 +3386,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) cleanup: virNetworkDefFree(def); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&net); return ret; } @@ -3415,8 +3415,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) cleanup: virNetworkDefFree(def); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&net); return ret; } @@ -3449,8 +3448,7 @@ static int testNetworkUndefine(virNetworkPtr network) ret = 0; cleanup: - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); return ret; } @@ -3527,8 +3525,7 @@ static int testNetworkCreate(virNetworkPtr network) ret = 0; cleanup: - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); return ret; } @@ -3556,8 +3553,7 @@ static int testNetworkDestroy(virNetworkPtr network) ret = 0; cleanup: - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); return ret; } @@ -5840,15 +5836,6 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn, return ret; } - -/* driver must be locked before calling */ -static void testObjectEventQueue(testConnPtr driver, - virObjectEventPtr event) -{ - virObjectEventStateQueue(driver->eventState, event); -} - - static int testConnectListAllDomains(virConnectPtr conn, virDomainPtr **domains, unsigned int flags) @@ -5959,11 +5946,7 @@ testDomainManagedSave(virDomainPtr dom, unsigned int flags) ret = 0; cleanup: virDomainObjEndAPI(&vm); - if (event) { - testDriverLock(privconn); - testObjectEventQueue(privconn, event); - testDriverUnlock(privconn); - } + testObjectEventQueueUnlocked(privconn, event); return ret; } @@ -6476,11 +6459,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, } virDomainObjEndAPI(&vm); } - if (event) { - testDriverLock(privconn); - testObjectEventQueue(privconn, event); - testDriverUnlock(privconn); - } + testObjectEventQueueUnlocked(privconn, event); virDomainSnapshotDefFree(def); return snapshot; } @@ -6720,8 +6699,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); goto load; } @@ -6800,8 +6778,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* Flush first event, now do transition 2 or 3 */ bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); @@ -6818,8 +6795,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, cleanup: if (event) { testObjectEventQueue(privconn, event); - if (event2) - testObjectEventQueue(privconn, event2); + testObjectEventQueue(privconn, event2); } else { virObjectUnref(event2); } -- 2.4.1
On 16.06.2015 19:43, Peter Krempa wrote:
Make testObjectEventQueue tolerant to NULL @event and move it so that it does not require a prototype. Additionally add testObjectEventQueueUnlocked that will lock @driver before sending the event.
Refactor the rest of the codebase to make use of the above features --- src/test/test_driver.c | 112 +++++++++++++++++++------------------------------ 1 file changed, 44 insertions(+), 68 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index dc6e49a..20d3c71 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -137,8 +137,6 @@ static const virNodeInfo defaultNodeInfo = {
@@ -3385,8 +3386,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml)
cleanup: virNetworkDefFree(def); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event);
Unlocked()
virNetworkObjEndAPI(&net); return ret; } @@ -3415,8 +3415,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml)
cleanup: virNetworkDefFree(def); - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event);
Unlocked()
virNetworkObjEndAPI(&net); return ret; } @@ -3449,8 +3448,7 @@ static int testNetworkUndefine(virNetworkPtr network) ret = 0;
cleanup: - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event);
Unlocked()
virNetworkObjEndAPI(&privnet); return ret; } @@ -3527,8 +3525,7 @@ static int testNetworkCreate(virNetworkPtr network) ret = 0;
cleanup: - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event);
Unlocked()
virNetworkObjEndAPI(&privnet); return ret; } @@ -3556,8 +3553,7 @@ static int testNetworkDestroy(virNetworkPtr network) ret = 0;
cleanup: - if (event) - testObjectEventQueue(privconn, event); + testObjectEventQueue(privconn, event);
Unlocked()
virNetworkObjEndAPI(&privnet); return ret; } @@ -5840,15 +5836,6 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn, return ret; }
ACK with those nits fixed. Michal
The test driver groups the API groups together, but some domain APIs were scattered around. --- src/test/test_driver.c | 225 +++++++++++++++++++++++++------------------------ 1 file changed, 113 insertions(+), 112 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 20d3c71..59e2031 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3231,6 +3231,119 @@ static int testDomainInterfaceStats(virDomainPtr domain, return ret; } +static int testConnectListAllDomains(virConnectPtr conn, + virDomainPtr **domains, + unsigned int flags) +{ + testConnPtr privconn = conn->privateData; + int ret; + + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ALL, -1); + + testDriverLock(privconn); + ret = virDomainObjListExport(privconn->domains, conn, domains, + NULL, flags); + testDriverUnlock(privconn); + + return ret; +} + +static char * +testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, + virStreamPtr st, + unsigned int screen ATTRIBUTE_UNUSED, + unsigned int flags) +{ + char *ret = NULL; + + virCheckFlags(0, NULL); + + if (VIR_STRDUP(ret, "image/png") < 0) + return NULL; + + if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY) < 0) + VIR_FREE(ret); + + return ret; +} + +static int +testDomainManagedSave(virDomainPtr dom, unsigned int flags) +{ + testConnPtr privconn = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virObjectEventPtr event = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | + VIR_DOMAIN_SAVE_RUNNING | + VIR_DOMAIN_SAVE_PAUSED, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + if (!vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot do managed save for transient domain")); + goto cleanup; + } + + testDomainShutdownState(dom, vm, VIR_DOMAIN_SHUTOFF_SAVED); + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_SAVED); + vm->hasManagedSave = true; + + ret = 0; + cleanup: + virDomainObjEndAPI(&vm); + testObjectEventQueueUnlocked(privconn, event); + + return ret; +} + +static int +testDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) +{ + virDomainObjPtr vm; + int ret; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + ret = vm->hasManagedSave; + + virDomainObjEndAPI(&vm); + return ret; +} + +static int +testDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) +{ + virDomainObjPtr vm; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(dom))) + return -1; + + vm->hasManagedSave = false; + + virDomainObjEndAPI(&vm); + return 0; +} + +/* + * Network APIs + */ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, const unsigned char *uuid) @@ -5836,23 +5949,6 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn, return ret; } -static int testConnectListAllDomains(virConnectPtr conn, - virDomainPtr **domains, - unsigned int flags) -{ - testConnPtr privconn = conn->privateData; - int ret; - - virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ALL, -1); - - testDriverLock(privconn); - ret = virDomainObjListExport(privconn->domains, conn, domains, - NULL, flags); - testDriverUnlock(privconn); - - return ret; -} - static int testNodeGetCPUMap(virConnectPtr conn, unsigned char **cpumap, @@ -5881,25 +5977,6 @@ testNodeGetCPUMap(virConnectPtr conn, return ret; } -static char * -testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, - virStreamPtr st, - unsigned int screen ATTRIBUTE_UNUSED, - unsigned int flags) -{ - char *ret = NULL; - - virCheckFlags(0, NULL); - - if (VIR_STRDUP(ret, "image/png") < 0) - return NULL; - - if (virFDStreamOpenFile(st, PKGDATADIR "/libvirtLogo.png", 0, 0, O_RDONLY) < 0) - VIR_FREE(ret); - - return ret; -} - static int testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, const char *arch, @@ -5910,82 +5987,6 @@ testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, return cpuGetModels(arch, models); } -static int -testDomainManagedSave(virDomainPtr dom, unsigned int flags) -{ - testConnPtr privconn = dom->conn->privateData; - virDomainObjPtr vm = NULL; - virObjectEventPtr event = NULL; - int ret = -1; - - virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | - VIR_DOMAIN_SAVE_RUNNING | - VIR_DOMAIN_SAVE_PAUSED, -1); - - if (!(vm = testDomObjFromDomain(dom))) - return -1; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot do managed save for transient domain")); - goto cleanup; - } - - testDomainShutdownState(dom, vm, VIR_DOMAIN_SHUTOFF_SAVED); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - VIR_DOMAIN_EVENT_STOPPED_SAVED); - vm->hasManagedSave = true; - - ret = 0; - cleanup: - virDomainObjEndAPI(&vm); - testObjectEventQueueUnlocked(privconn, event); - - return ret; -} - - -static int -testDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) -{ - virDomainObjPtr vm; - int ret; - - virCheckFlags(0, -1); - - if (!(vm = testDomObjFromDomain(dom))) - return -1; - - ret = vm->hasManagedSave; - - virDomainObjEndAPI(&vm); - return ret; -} - -static int -testDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) -{ - virDomainObjPtr vm; - - virCheckFlags(0, -1); - - if (!(vm = testDomObjFromDomain(dom))) - return -1; - - vm->hasManagedSave = false; - - virDomainObjEndAPI(&vm); - return 0; -} - - /* * Snapshot APIs */ -- 2.4.1
Since VIR_DOMAIN_AFFECT_CURRENT is 0 the flag check does not make sense as masking @flags with 0 will always equal to false. --- src/libvirt-domain.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 7e6d749..4d7b88a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7267,12 +7267,6 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, VIR_DOMAIN_AFFECT_CONFIG, error); - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT, - VIR_DOMAIN_AFFECT_LIVE, - error); - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT, - VIR_DOMAIN_AFFECT_CONFIG, - error); VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST, VIR_DOMAIN_AFFECT_CONFIG, error); -- 2.4.1
On Tue, Jun 16, 2015 at 07:43:18PM +0200, Peter Krempa wrote:
Since VIR_DOMAIN_AFFECT_CURRENT is 0 the flag check does not make sense as masking @flags with 0 will always equal to false. --- src/libvirt-domain.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 7e6d749..4d7b88a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -7267,12 +7267,6 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, VIR_DOMAIN_AFFECT_CONFIG, error);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT, - VIR_DOMAIN_AFFECT_LIVE, - error); - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT, - VIR_DOMAIN_AFFECT_CONFIG, - error); VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST, VIR_DOMAIN_AFFECT_CONFIG, error); -- 2.4.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ACK Pavel
Remove the bogous flag check and refactor the code by using virDomainObjGetDefs instead of virDomainObjGetPersistentDef. --- src/test/test_driver.c | 68 +++++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 45 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 59e2031..cfec122 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2509,6 +2509,7 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, { testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom = NULL; + virDomainDefPtr def; virDomainDefPtr persistentDef; int ret = -1, maxvcpus; @@ -2516,72 +2517,49 @@ testDomainSetVcpusFlags(virDomainPtr domain, unsigned int nrCpus, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - /* At least one of LIVE or CONFIG must be set. MAXIMUM cannot be - * mixed with LIVE. */ - if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == 0 || - (flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) == - (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) { - virReportError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); + if ((maxvcpus = testConnectGetMaxVcpus(domain->conn, NULL)) == -1) return -1; - } - if (!nrCpus || (maxvcpus = testConnectGetMaxVcpus(domain->conn, NULL)) < nrCpus) { + + if (nrCpus > maxvcpus) { virReportError(VIR_ERR_INVALID_ARG, - _("argument out of range: %d"), nrCpus); + _("requested cpu amount exceeds maximum supported amount " + "(%d > %d)"), nrCpus, maxvcpus); return -1; } if (!(privdom = testDomObjFromDomain(domain))) return -1; - if (!virDomainObjIsActive(privdom) && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot hotplug vcpus for an inactive domain")); + if (virDomainObjGetDefs(privdom, flags, &def, &persistentDef) < 0) goto cleanup; - } - - /* We allow more cpus in guest than host, but not more than the - * domain's starting limit. */ - if (!(flags & (VIR_DOMAIN_VCPU_MAXIMUM)) && - privdom->def->maxvcpus < maxvcpus) - maxvcpus = privdom->def->maxvcpus; - if (nrCpus > maxvcpus) { + if ((def && + def->maxvcpus < nrCpus) || + (persistentDef && + !(flags & VIR_DOMAIN_VCPU_MAXIMUM) && + persistentDef->maxvcpus < nrCpus)) { virReportError(VIR_ERR_INVALID_ARG, _("requested cpu amount exceeds maximum (%d > %d)"), nrCpus, maxvcpus); goto cleanup; } - if (!(persistentDef = virDomainObjGetPersistentDef(privconn->caps, - privconn->xmlopt, - privdom))) + if (def && + testDomainUpdateVCPUs(privconn, privdom, nrCpus, 0) < 0) goto cleanup; - switch (flags) { - case VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_CONFIG: - persistentDef->maxvcpus = nrCpus; - if (nrCpus < persistentDef->vcpus) - persistentDef->vcpus = nrCpus; - ret = 0; - break; - - case VIR_DOMAIN_AFFECT_CONFIG: - persistentDef->vcpus = nrCpus; - ret = 0; - break; - - case VIR_DOMAIN_AFFECT_LIVE: - ret = testDomainUpdateVCPUs(privconn, privdom, nrCpus, 0); - break; - - case VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG: - ret = testDomainUpdateVCPUs(privconn, privdom, nrCpus, 0); - if (ret == 0) + if (persistentDef) { + if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { + persistentDef->maxvcpus = nrCpus; + if (nrCpus < persistentDef->vcpus) + persistentDef->vcpus = nrCpus; + } else { persistentDef->vcpus = nrCpus; - break; + } } + ret = 0; + cleanup: virDomainObjEndAPI(&privdom); return ret; -- 2.4.1
On Tue, Jun 16, 2015 at 07:43:19PM +0200, Peter Krempa wrote:
Remove the bogous flag check and refactor the code by using
s/bogous/bogus
virDomainObjGetDefs instead of virDomainObjGetPersistentDef. --- src/test/test_driver.c | 68 +++++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 45 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 59e2031..cfec122 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c
- if (nrCpus > maxvcpus) { + if ((def && + def->maxvcpus < nrCpus) || + (persistentDef && + !(flags & VIR_DOMAIN_VCPU_MAXIMUM) && + persistentDef->maxvcpus < nrCpus)) { virReportError(VIR_ERR_INVALID_ARG, _("requested cpu amount exceeds maximum (%d > %d)"), nrCpus, maxvcpus);
The comparison uses {persistentD,d}ef->maxvcpus, but the error message shows maxvcpus.
goto cleanup; }
- if (!(persistentDef = virDomainObjGetPersistentDef(privconn->caps, - privconn->xmlopt, - privdom))) + if (def && + testDomainUpdateVCPUs(privconn, privdom, nrCpus, 0) < 0) goto cleanup;
ACK with those fixes. Jan
The test driver lock should not be acquired while a domain object lock is held. Tweak the lock ordering to avoid possible deadlock. --- src/test/test_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index cfec122..9e617a2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6603,13 +6603,13 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * and use of FORCE can cause multiple transitions. */ - if (!(vm = testDomObjFromSnapshot(snapshot))) - return -1; + testDriverLock(privconn); - if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) + if (!(vm = testDomObjFromDomainLocked(privconn, snapshot->domain))) goto cleanup; - testDriverLock(privconn); + if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) + goto cleanup; if (!vm->persistent && snap->def->state != VIR_DOMAIN_RUNNING && -- 2.4.1
On 16.06.2015 19:43, Peter Krempa wrote:
The test driver lock should not be acquired while a domain object lock is held. Tweak the lock ordering to avoid possible deadlock. --- src/test/test_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index cfec122..9e617a2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6603,13 +6603,13 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * and use of FORCE can cause multiple transitions. */
- if (!(vm = testDomObjFromSnapshot(snapshot))) - return -1; + testDriverLock(privconn);
- if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) + if (!(vm = testDomObjFromDomainLocked(privconn, snapshot->domain))) goto cleanup;
- testDriverLock(privconn); + if (!(snap = testSnapObjFromSnapshot(vm, snapshot))) + goto cleanup;
if (!vm->persistent && snap->def->state != VIR_DOMAIN_RUNNING &&
Fortunately, the public API already checked that @snapshot is not NULL and snapshot->domain is of virDomainClass class. ACK Michal
On 16.06.2015 19:43, Peter Krempa wrote:
Note that patch 1/8 of this series depends on patch 2/13 of the "vCPU pinning and related refactors - Part 1.5" series:
http://www.redhat.com/archives/libvir-list/2015-June/msg00678.html
Peter Krempa (8): test: Reuse virDomainObjGetOneDef in testDomainGetVcpusFlags test: Switch to reference counting with testDomObjFromDomain test: Refactor test driver domain object retrieval test: Refactor test driver event sending test: group domain APIs together lib: setvcpus: Remove bogous flag check test: Refactor testDomainSetVcpusFlags test: Fix lock ordering in testDomainRevertToSnapshot
src/libvirt-domain.c | 6 - src/test/test_driver.c | 941 +++++++++++++++++-------------------------------- 2 files changed, 324 insertions(+), 623 deletions(-)
Basically ACK series, but see my comments. Michal
participants (4)
-
Ján Tomko -
Michal Privoznik -
Pavel Hrdina -
Peter Krempa