[libvirt] [REPOST PATCH 0/2] vz - Clean/fix test driver usage of FindBy{UUID|ID}
Reposting patches 16-17 from: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html Reposting and using the vz prefix to make it clearer/cleaner as to what's changing (since the 20 patch series perhaps was a bit too daunting to look through it all). There are no changes since v1. John Ferlan (2): vz: Unify vzDomObjFromDomain{Ref} vz: Use virDomainObjListFindBy{UUID|ID}Ref src/vz/vz_driver.c | 119 ++++++++++++++++++++++++++--------------------------- src/vz/vz_sdk.c | 15 +++---- src/vz/vz_utils.c | 32 +------------- src/vz/vz_utils.h | 1 - 4 files changed, 68 insertions(+), 99 deletions(-) -- 2.13.6
Rather than have two API's doing different things for different callers, let's make one API that will always return a locked and ref counted object. That way, the callers will always know that they must call virDomainObjEndAPI and not have to decide whether they should call virObjectUnlock instead. This will make things consistent with LookupByName which returns the locked and ref counted object. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vz/vz_driver.c | 111 ++++++++++++++++++++++++++--------------------------- src/vz/vz_utils.c | 32 +-------------- src/vz/vz_utils.h | 1 - 3 files changed, 56 insertions(+), 88 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index a425bc8552..bcbccf6cc8 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -654,7 +654,7 @@ vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) vzDomObjPtr privdom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; if (virDomainGetInfoEnsureACL(domain->conn, dom->def) < 0) @@ -703,7 +703,7 @@ vzDomainGetOSType(virDomainPtr domain) ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(dom->def->os.type))); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -722,7 +722,7 @@ vzDomainIsPersistent(virDomainPtr domain) ret = 1; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -745,7 +745,7 @@ vzDomainGetState(virDomainPtr domain, ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -771,7 +771,7 @@ vzDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) ret = virDomainDefFormat(def, privconn->driver->caps, flags); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -791,7 +791,7 @@ vzDomainGetAutostart(virDomainPtr domain, int *autostart) ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -985,7 +985,7 @@ vzDomainGetVcpus(virDomainPtr domain, size_t i; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainGetVcpusEnsureACL(domain->conn, dom->def) < 0) @@ -1087,7 +1087,7 @@ vzDomainSuspend(virDomainPtr domain) int ret = -1; bool job = false; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSuspendEnsureACL(domain->conn, dom->def) < 0) @@ -1124,7 +1124,7 @@ vzDomainResume(virDomainPtr domain) int ret = -1; bool job = false; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainResumeEnsureACL(domain->conn, dom->def) < 0) @@ -1163,7 +1163,7 @@ vzDomainCreateWithFlags(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainCreateWithFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1202,7 +1202,7 @@ vzDomainDestroyFlags(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainDestroyFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1247,7 +1247,7 @@ vzDomainShutdownFlags(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainShutdownFlagsEnsureACL(domain->conn, dom->def, flags) < 0) @@ -1291,7 +1291,7 @@ vzDomainReboot(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainRebootEnsureACL(domain->conn, dom->def, flags) < 0) @@ -1334,7 +1334,7 @@ static int vzDomainIsActive(virDomainPtr domain) ret = virDomainObjIsActive(dom); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1357,7 +1357,7 @@ vzDomainUndefineFlags(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainUndefineFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1409,7 +1409,7 @@ vzDomainHasManagedSaveImage(virDomainPtr domain, unsigned int flags) ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1426,7 +1426,7 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags) virCheckFlags(VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainManagedSaveEnsureACL(domain->conn, dom->def) < 0) @@ -1469,7 +1469,7 @@ vzDomainManagedSaveRemove(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainManagedSaveRemoveEnsureACL(domain->conn, dom->def) < 0) @@ -1522,7 +1522,7 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (vzCheckConfigUpdateFlags(dom, &flags) < 0) @@ -1577,7 +1577,7 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - dom = vzDomObjFromDomainRef(domain); + dom = vzDomObjFromDomain(domain); if (dom == NULL) return -1; @@ -1634,7 +1634,7 @@ vzDomainSetUserPassword(virDomainPtr domain, bool job = false; virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSetUserPasswordEnsureACL(domain->conn, dom->def) < 0) @@ -1670,7 +1670,7 @@ static int vzDomainUpdateDeviceFlags(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainUpdateDeviceFlagsEnsureACL(domain->conn, dom->def, flags) < 0) @@ -1723,7 +1723,7 @@ vzDomainGetMaxMemory(virDomainPtr domain) ret = virDomainDefGetMemoryTotal(dom->def); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1784,7 +1784,7 @@ vzDomainBlockStats(virDomainPtr domain, virDomainObjPtr dom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainBlockStatsEnsureACL(domain->conn, dom->def) < 0) @@ -1851,7 +1851,7 @@ vzDomainBlockStatsFlags(virDomainPtr domain, /* We don't return strings, and thus trivially support this flag. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainBlockStatsFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1880,7 +1880,7 @@ vzDomainInterfaceStats(virDomainPtr domain, vzDomObjPtr privdom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainInterfaceStatsEnsureACL(domain->conn, dom->def) < 0) @@ -1907,7 +1907,7 @@ vzDomainMemoryStats(virDomainPtr domain, int ret = -1; virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainMemoryStatsEnsureACL(domain->conn, dom->def) < 0) @@ -1946,7 +1946,7 @@ vzDomainGetVcpusFlags(virDomainPtr domain, ret = virDomainDefGetVcpus(dom->def); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1973,7 +1973,7 @@ static int vzDomainIsUpdated(virDomainPtr domain) ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -2110,7 +2110,7 @@ static int vzDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (vzCheckConfigUpdateFlags(dom, &flags) < 0) @@ -2142,7 +2142,7 @@ static int vzDomainSetMemory(virDomainPtr domain, unsigned long memory) int ret = -1; bool job = false; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSetMemoryEnsureACL(domain->conn, dom->def) < 0) @@ -2217,7 +2217,7 @@ vzDomainSnapshotNum(virDomainPtr domain, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSnapshotNumEnsureACL(domain->conn, dom->def) < 0) @@ -2248,7 +2248,7 @@ vzDomainSnapshotListNames(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSnapshotListNamesEnsureACL(domain->conn, dom->def) < 0) @@ -2278,7 +2278,7 @@ vzDomainListAllSnapshots(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainListAllSnapshotsEnsureACL(domain->conn, dom->def) < 0) @@ -2308,7 +2308,7 @@ vzDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return NULL; if (virDomainSnapshotGetXMLDescEnsureACL(snapshot->domain->conn, dom->def, flags) < 0) @@ -2345,7 +2345,7 @@ vzDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotNumChildrenEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2380,7 +2380,7 @@ vzDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotListChildrenNamesEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2414,7 +2414,7 @@ vzDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotListAllChildrenEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2447,7 +2447,7 @@ vzDomainSnapshotLookupByName(virDomainPtr domain, virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return NULL; if (virDomainSnapshotLookupByNameEnsureACL(domain->conn, dom->def) < 0) @@ -2477,7 +2477,7 @@ vzDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainHasCurrentSnapshotEnsureACL(domain->conn, dom->def) < 0) @@ -2505,7 +2505,7 @@ vzDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return NULL; if (virDomainSnapshotGetParentEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2543,7 +2543,7 @@ vzDomainSnapshotCurrent(virDomainPtr domain, unsigned int flags) virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return NULL; if (virDomainSnapshotCurrentEnsureACL(domain->conn, dom->def) < 0) @@ -2577,7 +2577,7 @@ vzDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotIsCurrentEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2607,7 +2607,7 @@ vzDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotHasMetadataEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2645,7 +2645,7 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return NULL; if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, dom->def, flags) < 0) @@ -2708,7 +2708,7 @@ vzDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotDeleteEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2732,7 +2732,7 @@ vzDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainRevertToSnapshotEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2978,8 +2978,7 @@ vzDomainMigrateBegin3Params(virDomainPtr domain, cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return xml; } @@ -3322,7 +3321,7 @@ vzDomainMigratePerform3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, VZ_MIGRATION_PARAMETERS) < 0) return -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainMigratePerform3ParamsEnsureACL(domain->conn, dom->def) < 0) @@ -3445,7 +3444,7 @@ vzDomainGetJobInfo(virDomainPtr domain, virDomainJobInfoPtr info) ret = vzDomainGetJobInfoImpl(dom, info); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -3517,7 +3516,7 @@ vzDomainGetJobStats(virDomainPtr domain, ret = vzDomainJobInfoToParams(&info, type, params, nparams); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -3896,7 +3895,7 @@ vzDomainAbortJob(virDomainPtr domain) virDomainObjPtr dom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainAbortJobEnsureACL(domain->conn, dom->def) < 0) @@ -3919,7 +3918,7 @@ vzDomainReset(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainResetEnsureACL(domain->conn, dom->def) < 0) @@ -3951,7 +3950,7 @@ static int vzDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; if (vzCheckConfigUpdateFlags(dom, &flags) < 0) @@ -3994,7 +3993,7 @@ vzDomainBlockResize(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; if (virDomainBlockResizeEnsureACL(domain->conn, dom->def) < 0) diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 6fb27169a3..8f4e3e3474 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -63,43 +63,13 @@ static virDomainControllerType vz7ControllerTypes[] = {VIR_DOMAIN_CONTROLLER_TYP * @domain: Domain pointer that has to be looked up * * This function looks up @domain and returns the appropriate virDomainObjPtr - * that has to be unlocked by virObjectUnlock(). - * - * Returns the domain object without incremented reference counter which is locked - * on success, NULL otherwise. - */ -virDomainObjPtr -vzDomObjFromDomain(virDomainPtr domain) -{ - virDomainObjPtr vm; - vzConnPtr privconn = domain->conn->privateData; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - vzDriverPtr driver = privconn->driver; - - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - if (!vm) { - virUUIDFormat(domain->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s' (%s)"), - uuidstr, domain->name); - return NULL; - } - - return vm; -} - -/** - * vzDomObjFromDomainRef: - * @domain: Domain pointer that has to be looked up - * - * This function looks up @domain and returns the appropriate virDomainObjPtr * that has to be released by calling virDomainObjEndAPI(). * * Returns the domain object with incremented reference counter which is locked * on success, NULL otherwise. */ virDomainObjPtr -vzDomObjFromDomainRef(virDomainPtr domain) +vzDomObjFromDomain(virDomainPtr domain) { virDomainObjPtr vm; vzConnPtr privconn = domain->conn->privateData; diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index f9e9dee42a..40a1ce23dd 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -120,7 +120,6 @@ void* vzDomObjAlloc(void *opaque); void vzDomObjFree(void *p); virDomainObjPtr vzDomObjFromDomain(virDomainPtr domain); -virDomainObjPtr vzDomObjFromDomainRef(virDomainPtr domain); char * vzGetOutput(const char *binary, ...) ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; -- 2.13.6
On 02.04.2018 16:21, John Ferlan wrote:
Rather than have two API's doing different things for different callers, let's make one API that will always return a locked and ref counted object. That way, the callers will always know that they must call virDomainObjEndAPI and not have to decide whether they should call virObjectUnlock instead.
This will make things consistent with LookupByName which returns the locked and ref counted object.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vz/vz_driver.c | 111 ++++++++++++++++++++++++++--------------------------- src/vz/vz_utils.c | 32 +-------------- src/vz/vz_utils.h | 1 - 3 files changed, 56 insertions(+), 88 deletions(-)
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
For vzDomainLookupByID and vzDomainLookupByUUID let's return a locked and referenced @vm object so that callers can then use the common and more consistent virDomainObjEndAPI in order to handle cleanup rather than needing to know that the returned object is locked and calling virObjectUnlock. The LookupByName already returns the ref counted and locked object, so this will make things more consistent. Also adjust the prlsdkHandle{VmState|VmRemoved|Perf}Event APIs in the same manner. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vz/vz_driver.c | 8 ++++---- src/vz/vz_sdk.c | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index bcbccf6cc8..736424897a 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -578,7 +578,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) virDomainPtr ret = NULL; virDomainObjPtr dom; - dom = virDomainObjListFindByID(privconn->driver->domains, id); + dom = virDomainObjListFindByIDRef(privconn->driver->domains, id); if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, NULL); @@ -591,7 +591,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -602,7 +602,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr ret = NULL; virDomainObjPtr dom; - dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(privconn->driver->domains, uuid); if (dom == NULL) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -618,7 +618,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index a5b9f2da67..b8f13f88a7 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2144,7 +2144,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, virDomainEventType lvEventType = 0; int lvEventTypeDetails = 0; - dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (dom == NULL) return; @@ -2166,7 +2166,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, cleanup: PrlHandle_Free(eventParam); - virObjectUnlock(dom); + virObjectEndAPI(&dom); return; } @@ -2225,7 +2225,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL; - dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); /* domain was removed from the list from the libvirt * API function in current connection */ if (dom == NULL) @@ -2235,6 +2235,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); + virDomainObjEndAPI(&dom); return; } @@ -2246,7 +2247,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, virDomainObjPtr dom = NULL; vzDomObjPtr privdom = NULL; - if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { PrlHandle_Free(event); return; } @@ -2255,7 +2256,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, PrlHandle_Free(privdom->stats); privdom->stats = event; - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); } static void @@ -2269,7 +2270,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver, PRL_HANDLE param = PRL_INVALID_HANDLE; PRL_RESULT pret; - if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) return; pret = PrlEvent_GetParam(event, 0, ¶m); @@ -2283,7 +2284,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver, cleanup: PrlHandle_Free(param); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); } static PRL_RESULT -- 2.13.6
On 02.04.2018 16:21, John Ferlan wrote:
For vzDomainLookupByID and vzDomainLookupByUUID let's return a locked and referenced @vm object so that callers can then use the common and more consistent virDomainObjEndAPI in order to handle cleanup rather than needing to know that the returned object is locked and calling virObjectUnlock.
The LookupByName already returns the ref counted and locked object, so this will make things more consistent.
Also adjust the prlsdkHandle{VmState|VmRemoved|Perf}Event APIs in the same manner.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vz/vz_driver.c | 8 ++++---- src/vz/vz_sdk.c | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index bcbccf6cc8..736424897a 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -578,7 +578,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) virDomainPtr ret = NULL; virDomainObjPtr dom;
- dom = virDomainObjListFindByID(privconn->driver->domains, id); + dom = virDomainObjListFindByIDRef(privconn->driver->domains, id);
if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, NULL); @@ -591,7 +591,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
@@ -602,7 +602,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr ret = NULL; virDomainObjPtr dom;
- dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(privconn->driver->domains, uuid);
if (dom == NULL) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -618,7 +618,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index a5b9f2da67..b8f13f88a7 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2144,7 +2144,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, virDomainEventType lvEventType = 0; int lvEventTypeDetails = 0;
- dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (dom == NULL) return;
@@ -2166,7 +2166,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver,
cleanup: PrlHandle_Free(eventParam); - virObjectUnlock(dom); + virObjectEndAPI(&dom);
s/virObjectEndAPI/virDomainObjEndAPI/
return; }
@@ -2225,7 +2225,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL;
- dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); /* domain was removed from the list from the libvirt * API function in current connection */ if (dom == NULL) @@ -2235,6 +2235,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
virDomainObjListRemove(driver->domains, dom); + virDomainObjEndAPI(&dom);
virDomainObjListRemove unlocks dom object so we should only unref here. Also I doubt of usefullness of using ref/end api in vz_sdk.c as these functions are not API calls so it looks strange to see virDomainObjEndAPI name here.
return; }
@@ -2246,7 +2247,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, virDomainObjPtr dom = NULL; vzDomObjPtr privdom = NULL;
- if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { PrlHandle_Free(event); return; } @@ -2255,7 +2256,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, PrlHandle_Free(privdom->stats); privdom->stats = event;
- virObjectUnlock(dom); + virDomainObjEndAPI(&dom); }
static void @@ -2269,7 +2270,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver, PRL_HANDLE param = PRL_INVALID_HANDLE; PRL_RESULT pret;
- if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) return;
pret = PrlEvent_GetParam(event, 0, ¶m); @@ -2283,7 +2284,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver,
cleanup: PrlHandle_Free(param); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); }
static PRL_RESULT
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
On 04/20/2018 02:53 AM, Nikolay Shirokovskiy wrote:
On 02.04.2018 16:21, John Ferlan wrote:
For vzDomainLookupByID and vzDomainLookupByUUID let's return a locked and referenced @vm object so that callers can then use the common and more consistent virDomainObjEndAPI in order to handle cleanup rather than needing to know that the returned object is locked and calling virObjectUnlock.
The LookupByName already returns the ref counted and locked object, so this will make things more consistent.
Also adjust the prlsdkHandle{VmState|VmRemoved|Perf}Event APIs in the same manner.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vz/vz_driver.c | 8 ++++---- src/vz/vz_sdk.c | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index bcbccf6cc8..736424897a 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -578,7 +578,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) virDomainPtr ret = NULL; virDomainObjPtr dom;
- dom = virDomainObjListFindByID(privconn->driver->domains, id); + dom = virDomainObjListFindByIDRef(privconn->driver->domains, id);
if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, NULL); @@ -591,7 +591,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
@@ -602,7 +602,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr ret = NULL; virDomainObjPtr dom;
- dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(privconn->driver->domains, uuid);
if (dom == NULL) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -618,7 +618,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index a5b9f2da67..b8f13f88a7 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2144,7 +2144,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, virDomainEventType lvEventType = 0; int lvEventTypeDetails = 0;
- dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (dom == NULL) return;
@@ -2166,7 +2166,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver,
cleanup: PrlHandle_Free(eventParam); - virObjectUnlock(dom); + virObjectEndAPI(&dom);
s/virObjectEndAPI/virDomainObjEndAPI/
Oh right - it wasn't the first one of those too /-|... Just realized vz wasn't building on my host...
return; }
@@ -2225,7 +2225,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL;
- dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); /* domain was removed from the list from the libvirt * API function in current connection */ if (dom == NULL) @@ -2235,6 +2235,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
virDomainObjListRemove(driver->domains, dom); + virDomainObjEndAPI(&dom);
virDomainObjListRemove unlocks dom object so we should only unref here.
Actually, I'd prefer to follow what I've done elsewhere which is add the virObjectLock(dom); prior to the EndAPI call. Since we don't fail on our virObjectUnlock calls the fact that it wasn't locked before unlocking doesn't really register; however, upcoming changes to how *ListRemove works will do the "right" and "expected" thing, so having all the current consumers be consistent is preferred. Oh and by right and expected I mean since it receives a locked and reffed object, it should return a locked and reffed object to let the caller decide how to dispose.
Also I doubt of usefullness of using ref/end api in vz_sdk.c as these functions are not API calls so it looks strange to see virDomainObjEndAPI name here.
The long winded explanation just in case you've missed it in other related series... In the long run virDomainObjEndAPI is just a name chosen long ago - it ends up being the complementary call for the *FindBy* and *ListAdd calls. In the long run the *EndAPI just Unlocks and Unrefs the object. The *FindBy* calls were "inconsistent" w/r/t *Name always returned locked/reffed objects, while FindBy{UUID|ID} returned unreffed, but locked while the FindBy{UUID|ID}Ref would return the reffed/locked object. Having to "require" callers to know whether to use *EndAPI or ObjectUnlock makes things 'inconsistent' and has led to bugs. The *FindByName has been misused in a few places where only the Unlock was done meaning that object was probably never reaped. The *Add function returns a locked and 2X ref object. Some drivers add the additional ref (like done in prlsdkLoadDomain for @dom) and other drivers don't - it's been a process to make things consistent. For those that didn't take the additional ref, only virObjectLock was called after the *Add call. For those that did the *EndAPI was called. Thus leaving the object w/ 2 references (as it should have) once the Add is successful and for each FindBy call not altering that. The *Add code should return w/ 3 refs - one for each hash table add and one for existence. When the caller is "done" with it (*EndAPI), then the 2 refs remain because of the hash tables. Callers shouldn't have to know all this, but they do because they've found that Unref's at the wrong time lead to disappearing objects. All drivers handle it now, but in different ways. I'm trying to add a bit of consistency. This is all important because the *ListRemove code will remove 2 references when calling the virHashRemoveEntry for each hash table the object is in. Upon entry, ListRemove actually expects 3X ref and locked object. So removing the 2 refs is fine and the rest "fire dance" can really be avoidable. The problem is that it takes making the *FindBy* and *Add* callers to be consistent before cleaning up the ListRemove Thanks for the review - John BTW: It's not clear to me why getDomainJobResultHelper Unlocks and Locks @dom; however, I would think that could be dangerous if something else comes in and is able to lock/change @dom in the mean time. Seems to only matter for the prlsdkUpdateDomain though. Still if that code does what appears to be some sort of job wait and the code I'm unclear about is looking for job results, perhaps not an issue - just wasn't sure and figured I'd note/mention it.
return; }
@@ -2246,7 +2247,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, virDomainObjPtr dom = NULL; vzDomObjPtr privdom = NULL;
- if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { PrlHandle_Free(event); return; } @@ -2255,7 +2256,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, PrlHandle_Free(privdom->stats); privdom->stats = event;
- virObjectUnlock(dom); + virDomainObjEndAPI(&dom); }
static void @@ -2269,7 +2270,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver, PRL_HANDLE param = PRL_INVALID_HANDLE; PRL_RESULT pret;
- if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) return;
pret = PrlEvent_GetParam(event, 0, ¶m); @@ -2283,7 +2284,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver,
cleanup: PrlHandle_Free(param); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); }
static PRL_RESULT
Reviewed-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
On 20.04.2018 14:44, John Ferlan wrote:
On 04/20/2018 02:53 AM, Nikolay Shirokovskiy wrote:
On 02.04.2018 16:21, John Ferlan wrote:
For vzDomainLookupByID and vzDomainLookupByUUID let's return a locked and referenced @vm object so that callers can then use the common and more consistent virDomainObjEndAPI in order to handle cleanup rather than needing to know that the returned object is locked and calling virObjectUnlock.
The LookupByName already returns the ref counted and locked object, so this will make things more consistent.
Also adjust the prlsdkHandle{VmState|VmRemoved|Perf}Event APIs in the same manner.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vz/vz_driver.c | 8 ++++---- src/vz/vz_sdk.c | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index bcbccf6cc8..736424897a 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -578,7 +578,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) virDomainPtr ret = NULL; virDomainObjPtr dom;
- dom = virDomainObjListFindByID(privconn->driver->domains, id); + dom = virDomainObjListFindByIDRef(privconn->driver->domains, id);
if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, NULL); @@ -591,7 +591,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
@@ -602,7 +602,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr ret = NULL; virDomainObjPtr dom;
- dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(privconn->driver->domains, uuid);
if (dom == NULL) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -618,7 +618,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id);
cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; }
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index a5b9f2da67..b8f13f88a7 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2144,7 +2144,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, virDomainEventType lvEventType = 0; int lvEventTypeDetails = 0;
- dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (dom == NULL) return;
@@ -2166,7 +2166,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver,
cleanup: PrlHandle_Free(eventParam); - virObjectUnlock(dom); + virObjectEndAPI(&dom);
s/virObjectEndAPI/virDomainObjEndAPI/
Oh right - it wasn't the first one of those too /-|... Just realized vz wasn't building on my host...
return; }
@@ -2225,7 +2225,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL;
- dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); /* domain was removed from the list from the libvirt * API function in current connection */ if (dom == NULL) @@ -2235,6 +2235,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED);
virDomainObjListRemove(driver->domains, dom); + virDomainObjEndAPI(&dom);
virDomainObjListRemove unlocks dom object so we should only unref here.
Actually, I'd prefer to follow what I've done elsewhere which is add the virObjectLock(dom); prior to the EndAPI call. Since we don't fail on our
Ok
virObjectUnlock calls the fact that it wasn't locked before unlocking doesn't really register; however, upcoming changes to how *ListRemove
We use 'PTHREAD_MUTEX_NORMAL' mutexes and unlocking unlocked is undefined behaviour for such mutexes...
works will do the "right" and "expected" thing, so having all the current consumers be consistent is preferred.
Oh and by right and expected I mean since it receives a locked and reffed object, it should return a locked and reffed object to let the caller decide how to dispose.
Also I doubt of usefullness of using ref/end api in vz_sdk.c as these functions are not API calls so it looks strange to see virDomainObjEndAPI name here.
The long winded explanation just in case you've missed it in other related series...
In the long run virDomainObjEndAPI is just a name chosen long ago - it ends up being the complementary call for the *FindBy* and *ListAdd calls. In the long run the *EndAPI just Unlocks and Unrefs the object.
The *FindBy* calls were "inconsistent" w/r/t *Name always returned locked/reffed objects, while FindBy{UUID|ID} returned unreffed, but locked while the FindBy{UUID|ID}Ref would return the reffed/locked object. Having to "require" callers to know whether to use *EndAPI or ObjectUnlock makes things 'inconsistent' and has led to bugs. The *FindByName has been misused in a few places where only the Unlock was done meaning that object was probably never reaped.
The *Add function returns a locked and 2X ref object. Some drivers add the additional ref (like done in prlsdkLoadDomain for @dom) and other drivers don't - it's been a process to make things consistent. For those that didn't take the additional ref, only virObjectLock was called after the *Add call. For those that did the *EndAPI was called. Thus leaving the object w/ 2 references (as it should have) once the Add is successful and for each FindBy call not altering that. The *Add code should return w/ 3 refs - one for each hash table add and one for existence. When the caller is "done" with it (*EndAPI), then the 2 refs remain because of the hash tables. Callers shouldn't have to know all this, but they do because they've found that Unref's at the wrong time lead to disappearing objects. All drivers handle it now, but in different ways. I'm trying to add a bit of consistency.
This is all important because the *ListRemove code will remove 2 references when calling the virHashRemoveEntry for each hash table the object is in. Upon entry, ListRemove actually expects 3X ref and locked object. So removing the 2 refs is fine and the rest "fire dance" can really be avoidable.
The problem is that it takes making the *FindBy* and *Add* callers to be consistent before cleaning up the ListRemove
Ok. Thanx for explanation!
Thanks for the review -
John
BTW: It's not clear to me why getDomainJobResultHelper Unlocks and Locks @dom; however, I would think that could be dangerous if something else comes in and is able to lock/change @dom in the mean time. Seems to only matter for the prlsdkUpdateDomain though. Still if that code does what appears to be some sort of job wait and the code I'm unclear about is looking for job results, perhaps not an issue - just wasn't sure and figured I'd note/mention it.
It is just like unlock/lock in qemuDomainObjEnterMonitor/qemuDomainObjExitMonitor. Waiting for a job result can take time and holding a domain lock all this time is undesirable. Domain lock is not meant to be hold for a long time or event simple domain list operation will hang. To protect domain from unserialized changes while lock is dropped there is a job condition just like in qemu driver. Nikolay
ping? Changes are less about vz driver and more about domain object manipulation consistency. Tks, John On 04/02/2018 09:21 AM, John Ferlan wrote:
Reposting patches 16-17 from: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html
Reposting and using the vz prefix to make it clearer/cleaner as to what's changing (since the 20 patch series perhaps was a bit too daunting to look through it all).
There are no changes since v1.
John Ferlan (2): vz: Unify vzDomObjFromDomain{Ref} vz: Use virDomainObjListFindBy{UUID|ID}Ref
src/vz/vz_driver.c | 119 ++++++++++++++++++++++++++--------------------------- src/vz/vz_sdk.c | 15 +++---- src/vz/vz_utils.c | 32 +------------- src/vz/vz_utils.h | 1 - 4 files changed, 68 insertions(+), 99 deletions(-)
ping^2. Second of the remaining FindBy type changes... Tks, John On 04/11/2018 12:05 PM, John Ferlan wrote:
ping?
Changes are less about vz driver and more about domain object manipulation consistency.
Tks,
John
On 04/02/2018 09:21 AM, John Ferlan wrote:
Reposting patches 16-17 from: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html
Reposting and using the vz prefix to make it clearer/cleaner as to what's changing (since the 20 patch series perhaps was a bit too daunting to look through it all).
There are no changes since v1.
John Ferlan (2): vz: Unify vzDomObjFromDomain{Ref} vz: Use virDomainObjListFindBy{UUID|ID}Ref
src/vz/vz_driver.c | 119 ++++++++++++++++++++++++++--------------------------- src/vz/vz_sdk.c | 15 +++---- src/vz/vz_utils.c | 32 +------------- src/vz/vz_utils.h | 1 - 4 files changed, 68 insertions(+), 99 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy