8:21 a.m.
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
8:21 a.m.
New subject: [libvirt] [REPOST PATCH 1/2] vz: Unify vzDomObjFromDomain{Ref}
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(a)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
1:38 a.m.
New subject: [libvirt] [REPOST PATCH 1/2] vz: Unify vzDomObjFromDomain{Ref}
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(a)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(a)virtuozzo.com>
8:21 a.m.
New subject: [libvirt] [REPOST PATCH 2/2] vz: Use virDomainObjListFindBy{UUID|ID}Ref
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(a)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
1:53 a.m.
New subject: [libvirt] [REPOST PATCH 2/2] vz: Use virDomainObjListFindBy{UUID|ID}Ref
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(a)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(a)virtuozzo.com>
6:44 a.m.
New subject: [libvirt] [REPOST PATCH 2/2] vz: Use virDomainObjListFindBy{UUID|ID}Ref
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(a)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(a)virtuozzo.com>
7:07 a.m.
New subject: [libvirt] [REPOST PATCH 2/2] vz: Use virDomainObjListFindBy{UUID|ID}Ref
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(a)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
11:05 a.m.
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(-)
3:23 p.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2409
days inactive
2427
days old
8 comments
2 participants
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy