12:14 p.m.
As discussed here:
https://www.redhat.com/archives/libvir-list/2015-April/msg01135.html
Michal Privoznik (5):
virDomainObjListAddLocked: s/false/NULL/ for @oldDef
virDomainObjListNew: Use virObjectFreeHashData
Introduce virDomainObjEndAPI
virDomainObjListFindByName: Return referenced object
virDomainObjList: Introduce yet another hash table
src/bhyve/bhyve_driver.c | 3 +-
src/conf/domain_conf.c | 83 +++++++++++++++++++++++------------
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/libxl/libxl_driver.c | 10 ++---
src/lxc/lxc_driver.c | 3 +-
src/openvz/openvz_driver.c | 11 +++--
src/parallels/parallels_driver.c | 3 +-
src/qemu/qemu_domain.c | 7 +--
src/qemu/qemu_driver.c | 14 ++----
src/test/test_driver.c | 93 +++++++++++++---------------------------
src/uml/uml_driver.c | 15 +++----
12 files changed, 110 insertions(+), 135 deletions(-)
--
2.0.5
12:14 p.m.
New subject: [libvirt] [PATCH 1/5] virDomainObjListAddLocked: s/false/NULL/ for @oldDef
It's a pointer after all. We should initialize it to NULL instead of
false.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9aad782..ccd3bdf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2490,7 +2490,7 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
char uuidstr[VIR_UUID_STRING_BUFLEN];
if (oldDef)
- *oldDef = false;
+ *oldDef = NULL;
virUUIDFormat(def->uuid, uuidstr);
--
2.0.5
2:48 a.m.
New subject: [libvirt] [PATCH 1/5] virDomainObjListAddLocked: s/false/NULL/ for @oldDef
On Thu, Apr 23, 2015 at 19:14:54 +0200, Michal Privoznik wrote:
It's a pointer after all. We should initialize it to NULL instead
of
false.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK,
Peter
12:14 p.m.
New subject: [libvirt] [PATCH 2/5] virDomainObjListNew: Use virObjectFreeHashData
There's no point in duplicating virObjectFreeHashData() in a separate
function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ccd3bdf..aad4ec0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1032,13 +1032,6 @@
virDomainDeviceDefCheckUnsupportedMemoryDevice(virDomainDeviceDefPtr dev)
}
-static void
-virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED)
-{
- virDomainObjPtr obj = payload;
- virObjectUnref(obj);
-}
-
virDomainObjListPtr virDomainObjListNew(void)
{
virDomainObjListPtr doms;
@@ -1049,7 +1042,7 @@ virDomainObjListPtr virDomainObjListNew(void)
if (!(doms = virObjectLockableNew(virDomainObjListClass)))
return NULL;
- if (!(doms->objs = virHashCreate(50, virDomainObjListDataFree))) {
+ if (!(doms->objs = virHashCreate(50, virObjectFreeHashData))) {
virObjectUnref(doms);
return NULL;
}
--
2.0.5
2:48 a.m.
New subject: [libvirt] [PATCH 2/5] virDomainObjListNew: Use virObjectFreeHashData
On Thu, Apr 23, 2015 at 19:14:55 +0200, Michal Privoznik wrote:
There's no point in duplicating virObjectFreeHashData() in a
separate
function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
ACK,
Peter
12:14 p.m.
New subject: [libvirt] [PATCH 3/5] Introduce virDomainObjEndAPI
This is basically turning qemuDomObjEndAPI into a more general
function. Other drivers which gets a reference to domain objects may
benefit from this function too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
src/conf/domain_conf.h | 2 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 7 +------
4 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index aad4ec0..686c614 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2460,6 +2460,28 @@ void virDomainObjAssignDef(virDomainObjPtr domain,
}
+/**
+ * virDomainObjEndAPI:
+ * @vm: domain object
+ *
+ * Finish working with a domain object in an API. This function
+ * clears whatever was left of a domain that was gathered using
+ * virDomainObjListFindByUUIDRef(). Currently that means only unlocking and
+ * decrementing the reference counter of that domain. And in order to
+ * make sure the caller does not access the domain, the pointer is
+ * cleared.
+ */
+void
+virDomainObjEndAPI(virDomainObjPtr *vm)
+{
+ if (!*vm)
+ return;
+
+ virObjectUnlock(*vm);
+ virObjectUnref(*vm);
+ *vm = NULL;
+}
+
/*
*
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 25d3ee6..9955052 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2397,6 +2397,8 @@ virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr
doms,
virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
const char *name);
+void virDomainObjEndAPI(virDomainObjPtr *vm);
+
bool virDomainObjTaint(virDomainObjPtr obj,
virDomainTaintFlags taint);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8c50ea2..e6555f1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -374,6 +374,7 @@ virDomainNostateReasonTypeFromString;
virDomainNostateReasonTypeToString;
virDomainObjAssignDef;
virDomainObjCopyPersistentDef;
+virDomainObjEndAPI;
virDomainObjFormat;
virDomainObjGetMetadata;
virDomainObjGetPersistentDef;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1368386..b588c11 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2988,12 +2988,7 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
void
qemuDomObjEndAPI(virDomainObjPtr *vm)
{
- if (!*vm)
- return;
-
- virObjectUnlock(*vm);
- virObjectUnref(*vm);
- *vm = NULL;
+ virDomainObjEndAPI(vm);
}
--
2.0.5
3:28 a.m.
New subject: [libvirt] [PATCH 3/5] Introduce virDomainObjEndAPI
On Thu, Apr 23, 2015 at 19:14:56 +0200, Michal Privoznik wrote:
This is basically turning qemuDomObjEndAPI into a more general
function. Other drivers which gets a reference to domain objects may
benefit from this function too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
src/conf/domain_conf.h | 2 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_domain.c | 7 +------
4 files changed, 26 insertions(+), 6 deletions(-)
ACK, although I think that sed-ing s/qemuDomObjEndAPI/virDomainObjEndAPI/
could be done at this point too.
Peter
12:14 p.m.
New subject: [libvirt] [PATCH 4/5] virDomainObjListFindByName: Return referenced object
Every domain that grabs a domain object to work over should
reference it to make sure it won't disappear meanwhile.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/bhyve/bhyve_driver.c | 3 +-
src/conf/domain_conf.c | 1 +
src/libxl/libxl_driver.c | 10 ++---
src/lxc/lxc_driver.c | 3 +-
src/openvz/openvz_driver.c | 11 +++--
src/parallels/parallels_driver.c | 3 +-
src/qemu/qemu_driver.c | 14 ++----
src/test/test_driver.c | 93 +++++++++++++---------------------------
src/uml/uml_driver.c | 15 +++----
9 files changed, 51 insertions(+), 102 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index edbf1e4..dc76cf7 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -797,8 +797,7 @@ static virDomainPtr bhyveDomainLookupByName(virConnectPtr conn,
dom->id = vm->def->id;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return dom;
}
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 686c614..6666d03 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1157,6 +1157,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr
doms,
virDomainObjPtr obj;
virObjectLock(doms);
obj = virHashSearch(doms->objs, virDomainObjListSearchName, name);
+ virObjectRef(obj);
if (obj) {
virObjectLock(obj);
if (obj->removing) {
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 6a54c73..393f8bd 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1001,8 +1001,7 @@ libxlDomainLookupByName(virConnectPtr conn, const char *name)
dom->id = vm->def->id;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return dom;
}
@@ -4955,12 +4954,12 @@ libxlDomainMigrateFinish3Params(virConnectPtr dconn,
}
if (virDomainMigrateFinish3ParamsEnsureACL(dconn, vm->def) < 0) {
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return NULL;
}
if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return NULL;
}
@@ -4969,8 +4968,7 @@ libxlDomainMigrateFinish3Params(virConnectPtr dconn,
if (!libxlDomainObjEndJob(driver, vm))
vm = NULL;
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return ret;
}
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 38d9bed..188ff80 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -321,8 +321,7 @@ static virDomainPtr lxcDomainLookupByName(virConnectPtr conn,
dom->id = vm->def->id;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return dom;
}
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 1bb8973..10d94ff 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -427,8 +427,7 @@ static virDomainPtr openvzDomainLookupByName(virConnectPtr conn,
dom->id = vm->def->id;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return dom;
}
@@ -1007,6 +1006,7 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int fla
virReportError(VIR_ERR_OPERATION_FAILED,
_("Already an OPENVZ VM active with the id
'%s'"),
vmdef->name);
+ virDomainObjEndAPI(&vm);
goto cleanup;
}
if (!(vm = virDomainObjListAdd(driver->domains, vmdef,
@@ -1103,6 +1103,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml,
virReportError(VIR_ERR_OPERATION_FAILED,
_("Already an OPENVZ VM defined with the id
'%s'"),
vmdef->name);
+ virDomainObjEndAPI(&vm);
goto cleanup;
}
if (!(vm = virDomainObjListAdd(driver->domains,
@@ -1208,8 +1209,7 @@ openvzDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
ret = 0;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return ret;
}
@@ -2508,8 +2508,7 @@ openvzDomainMigrateFinish3Params(virConnectPtr dconn,
dom->id = vm->def->id;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return dom;
}
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index 07f1311..1ddc14e 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -526,8 +526,7 @@ parallelsDomainLookupByName(virConnectPtr conn, const char *name)
ret->id = dom->def->id;
cleanup:
- if (dom)
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
return ret;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 82f34ec..47ada4b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1511,8 +1511,7 @@ static virDomainPtr qemuDomainLookupByName(virConnectPtr conn,
if (dom) dom->id = vm->def->id;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ qemuDomObjEndAPI(&vm);
return dom;
}
@@ -12287,11 +12286,10 @@ qemuDomainMigrateFinish2(virConnectPtr dconn,
}
if (virDomainMigrateFinish2EnsureACL(dconn, vm->def) < 0) {
- virObjectUnlock(vm);
+ qemuDomObjEndAPI(&vm);
goto cleanup;
}
- virObjectRef(vm);
/* Do not use cookies in v2 protocol, since the cookie
* length was not sufficiently large, causing failures
* migrating between old & new libvirtd
@@ -12702,12 +12700,10 @@ qemuDomainMigrateFinish3(virConnectPtr dconn,
}
if (virDomainMigrateFinish3EnsureACL(dconn, vm->def) < 0) {
- virObjectUnlock(vm);
+ qemuDomObjEndAPI(&vm);
return NULL;
}
- virObjectRef(vm);
-
return qemuMigrationFinish(driver, dconn, vm,
cookiein, cookieinlen,
cookieout, cookieoutlen,
@@ -12747,12 +12743,10 @@ qemuDomainMigrateFinish3Params(virConnectPtr dconn,
}
if (virDomainMigrateFinish3ParamsEnsureACL(dconn, vm->def) < 0) {
- virObjectUnlock(vm);
+ qemuDomObjEndAPI(&vm);
return NULL;
}
- virObjectRef(vm);
-
return qemuMigrationFinish(driver, dconn, vm,
cookiein, cookieinlen,
cookieout, cookieoutlen,
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 133805c..231c72a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1859,8 +1859,7 @@ static virDomainPtr testDomainLookupByName(virConnectPtr conn,
ret->id = dom->def->id;
cleanup:
- if (dom)
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
return ret;
}
@@ -1902,13 +1901,11 @@ static int testDomainDestroy(virDomainPtr domain)
if (!privdom->persistent) {
virDomainObjListRemove(privconn->domains,
privdom);
- privdom = NULL;
}
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event)
testObjectEventQueue(privconn, event);
testDriverUnlock(privconn);
@@ -1946,8 +1943,7 @@ static int testDomainResume(virDomainPtr domain)
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event) {
testDriverLock(privconn);
testObjectEventQueue(privconn, event);
@@ -1988,8 +1984,7 @@ static int testDomainSuspend(virDomainPtr domain)
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event) {
testDriverLock(privconn);
@@ -2032,13 +2027,11 @@ static int testDomainShutdownFlags(virDomainPtr domain,
if (!privdom->persistent) {
virDomainObjListRemove(privconn->domains,
privdom);
- privdom = NULL;
}
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event)
testObjectEventQueue(privconn, event);
testDriverUnlock(privconn);
@@ -2107,14 +2100,12 @@ static int testDomainReboot(virDomainPtr domain,
if (!privdom->persistent) {
virDomainObjListRemove(privconn->domains,
privdom);
- privdom = NULL;
}
}
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event)
testObjectEventQueue(privconn, event);
testDriverUnlock(privconn);
@@ -2153,8 +2144,7 @@ static int testDomainGetInfo(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -2184,8 +2174,7 @@ testDomainGetState(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -2271,7 +2260,6 @@ testDomainSaveFlags(virDomainPtr domain, const char *path,
if (!privdom->persistent) {
virDomainObjListRemove(privconn->domains,
privdom);
- privdom = NULL;
}
ret = 0;
@@ -2285,8 +2273,7 @@ testDomainSaveFlags(virDomainPtr domain, const char *path,
VIR_FORCE_CLOSE(fd);
unlink(path);
}
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event)
testObjectEventQueue(privconn, event);
testDriverUnlock(privconn);
@@ -2461,15 +2448,13 @@ static int testDomainCoreDumpWithFormat(virDomainPtr domain,
if (!privdom->persistent) {
virDomainObjListRemove(privconn->domains,
privdom);
- privdom = NULL;
}
}
ret = 0;
cleanup:
VIR_FORCE_CLOSE(fd);
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event)
testObjectEventQueue(privconn, event);
testDriverUnlock(privconn);
@@ -2517,8 +2502,7 @@ testDomainGetMaxMemory(virDomainPtr domain)
ret = virDomainDefGetMemoryActual(privdom->def);
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -2544,8 +2528,7 @@ static int testDomainSetMaxMemory(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -2575,8 +2558,7 @@ static int testDomainSetMemory(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -2806,8 +2788,7 @@ static int testDomainGetVcpus(virDomainPtr domain,
ret = maxinfo;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -2863,8 +2844,7 @@ static int testDomainPinVcpu(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -2894,8 +2874,7 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int
flags)
virDomainDefFormatConvertXMLFlags(flags));
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3014,8 +2993,7 @@ static char *testDomainGetMetadata(virDomainPtr dom,
privconn->xmlopt, flags);
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3048,8 +3026,7 @@ static int testDomainSetMetadata(virDomainPtr dom,
NULL, NULL, flags);
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3118,8 +3095,7 @@ static int testDomainCreateWithFlags(virDomainPtr domain, unsigned
int flags)
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event)
testObjectEventQueue(privconn, event);
testDriverUnlock(privconn);
@@ -3188,14 +3164,12 @@ static int testDomainUndefineFlags(virDomainPtr domain,
} else {
virDomainObjListRemove(privconn->domains,
privdom);
- privdom = NULL;
}
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
if (event)
testObjectEventQueue(privconn, event);
testDriverUnlock(privconn);
@@ -3228,8 +3202,7 @@ static int testDomainGetAutostart(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3255,8 +3228,7 @@ static int testDomainSetAutostart(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3305,8 +3277,7 @@ testDomainGetSchedulerParametersFlags(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3356,8 +3327,7 @@ testDomainSetSchedulerParametersFlags(virDomainPtr domain,
ret = 0;
cleanup:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3417,8 +3387,7 @@ static int testDomainBlockStats(virDomainPtr domain,
ret = 0;
error:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -3476,8 +3445,7 @@ static int testDomainInterfaceStats(virDomainPtr domain,
ret = 0;
error:
- if (privdom)
- virObjectUnlock(privdom);
+ virDomainObjEndAPI(&privdom);
return ret;
}
@@ -6211,8 +6179,7 @@ testDomainManagedSave(virDomainPtr dom, unsigned int flags)
ret = 0;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
if (event) {
testDriverLock(privconn);
testObjectEventQueue(privconn, event);
@@ -6242,8 +6209,7 @@ testDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
ret = vm->hasManagedSave;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
testDriverUnlock(privconn);
return ret;
}
@@ -6268,8 +6234,7 @@ testDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags)
vm->hasManagedSave = false;
ret = 0;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
testDriverUnlock(privconn);
return ret;
}
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 9cee541..f8a84e4 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -339,7 +339,7 @@ umlInotifyEvent(int watch,
if (e.mask & IN_DELETE) {
VIR_DEBUG("Got inotify domain shutdown '%s'", name);
if (!virDomainObjIsActive(dom)) {
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
continue;
}
@@ -351,17 +351,16 @@ umlInotifyEvent(int watch,
if (!dom->persistent) {
virDomainObjListRemove(driver->domains,
dom);
- dom = NULL;
}
} else if (e.mask & (IN_CREATE | IN_MODIFY)) {
VIR_DEBUG("Got inotify domain startup '%s'", name);
if (virDomainObjIsActive(dom)) {
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
continue;
}
if (umlReadPidFile(driver, dom) < 0) {
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
continue;
}
@@ -385,7 +384,6 @@ umlInotifyEvent(int watch,
if (!dom->persistent) {
virDomainObjListRemove(driver->domains,
dom);
- dom = NULL;
}
} else if (umlIdentifyChrPTY(driver, dom) < 0) {
VIR_WARN("Could not identify character devices for new
domain");
@@ -398,12 +396,10 @@ umlInotifyEvent(int watch,
if (!dom->persistent) {
virDomainObjListRemove(driver->domains,
dom);
- dom = NULL;
}
}
}
- if (dom)
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
if (event) {
umlDomainEventQueue(driver, event);
event = NULL;
@@ -1448,8 +1444,7 @@ static virDomainPtr umlDomainLookupByName(virConnectPtr conn,
if (dom) dom->id = vm->def->id;
cleanup:
- if (vm)
- virObjectUnlock(vm);
+ virDomainObjEndAPI(&vm);
return dom;
}
--
2.0.5
4:13 a.m.
New subject: [libvirt] [PATCH 4/5] virDomainObjListFindByName: Return referenced object
On Thu, Apr 23, 2015 at 19:14:57 +0200, Michal Privoznik wrote:
Every domain that grabs a domain object to work over should
reference it to make sure it won't disappear meanwhile.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/bhyve/bhyve_driver.c | 3 +-
src/conf/domain_conf.c | 1 +
src/libxl/libxl_driver.c | 10 ++---
src/lxc/lxc_driver.c | 3 +-
src/openvz/openvz_driver.c | 11 +++--
src/parallels/parallels_driver.c | 3 +-
src/qemu/qemu_driver.c | 14 ++----
src/test/test_driver.c | 93 +++++++++++++---------------------------
src/uml/uml_driver.c | 15 +++----
9 files changed, 51 insertions(+), 102 deletions(-)
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 686c614..6666d03 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1157,6 +1157,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr
doms,
virDomainObjPtr obj;
virObjectLock(doms);
obj = virHashSearch(doms->objs, virDomainObjListSearchName, name);
+ virObjectRef(obj);
if (obj) {
virObjectLock(obj);
if (obj->removing) {
Here this code that is below in the context checks if the @removing flag
is set and if so the function returns NULL. With your addition the
reference you've added wouldn't be removed.
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 1bb8973..10d94ff 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
...
@@ -1007,6 +1006,7 @@ openvzDomainDefineXMLFlags(virConnectPtr conn,
const char *xml, unsigned int fla
virReportError(VIR_ERR_OPERATION_FAILED,
_("Already an OPENVZ VM active with the id
'%s'"),
vmdef->name);
+ virDomainObjEndAPI(&vm);
goto cleanup;
}
if (!(vm = virDomainObjListAdd(driver->domains, vmdef,
The whole piece of code that looks up the domain and reports the error
above could be removed as virDomainObjListAdd does the same check.
@@ -1103,6 +1103,7 @@ openvzDomainCreateXML(virConnectPtr conn, const
char *xml,
virReportError(VIR_ERR_OPERATION_FAILED,
_("Already an OPENVZ VM defined with the id
'%s'"),
vmdef->name);
+ virDomainObjEndAPI(&vm);
goto cleanup;
}
if (!(vm = virDomainObjListAdd(driver->domains,
Same here. Not worth changing though probably.
...
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 9cee541..f8a84e4 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -339,7 +339,7 @@ umlInotifyEvent(int watch,
if (e.mask & IN_DELETE) {
VIR_DEBUG("Got inotify domain shutdown '%s'", name);
if (!virDomainObjIsActive(dom)) {
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
continue;
}
@@ -351,17 +351,16 @@ umlInotifyEvent(int watch,
if (!dom->persistent) {
virDomainObjListRemove(driver->domains,
dom);
- dom = NULL;
}
This now leaves a single statement in an if with braces.
} else if (e.mask & (IN_CREATE | IN_MODIFY)) {
VIR_DEBUG("Got inotify domain startup '%s'", name);
if (virDomainObjIsActive(dom)) {
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
continue;
}
if (umlReadPidFile(driver, dom) < 0) {
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
continue;
}
@@ -385,7 +384,6 @@ umlInotifyEvent(int watch,
if (!dom->persistent) {
virDomainObjListRemove(driver->domains,
dom);
- dom = NULL;
Again, single statement in an if with braces
}
} else if (umlIdentifyChrPTY(driver, dom) < 0) {
VIR_WARN("Could not identify character devices for new
domain");
@@ -398,12 +396,10 @@ umlInotifyEvent(int watch,
if (!dom->persistent) {
virDomainObjListRemove(driver->domains,
dom);
- dom = NULL;
Here too
}
}
}
- if (dom)
- virObjectUnlock(dom);
+ virDomainObjEndAPI(&dom);
if (event) {
umlDomainEventQueue(driver, event);
event = NULL;
ACK, if you fix the broken reference counting in case the domain is
being removed.
Peter
12:14 p.m.
New subject: [libvirt] [PATCH 5/5] virDomainObjList: Introduce yet another hash table
This hash table will contain the same data as already existing one.
The only difference is that while the first table uses domain uuid as
key, the new table uses domain name. This will allow much faster (and
lockless) lookups by domain name.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 51 +++++++++++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6666d03..41963cc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -65,6 +65,10 @@ struct _virDomainObjList {
/* uuid string -> virDomainObj mapping
* for O(1), lockless lookup-by-uuid */
virHashTable *objs;
+
+ /* name -> virDomainObj mapping for O(1),
+ * lockless lookup-by-name */
+ virHashTable *objsName;
};
@@ -1042,7 +1046,8 @@ virDomainObjListPtr virDomainObjListNew(void)
if (!(doms = virObjectLockableNew(virDomainObjListClass)))
return NULL;
- if (!(doms->objs = virHashCreate(50, virObjectFreeHashData))) {
+ if (!(doms->objs = virHashCreate(50, virObjectFreeHashData)) ||
+ !(doms->objsName = virHashCreate(50, virObjectFreeHashData))) {
virObjectUnref(doms);
return NULL;
}
@@ -1056,6 +1061,7 @@ static void virDomainObjListDispose(void *obj)
virDomainObjListPtr doms = obj;
virHashFree(doms->objs);
+ virHashFree(doms->objsName);
}
@@ -1137,27 +1143,15 @@ virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
return virDomainObjListFindByUUIDInternal(doms, uuid, true);
}
-static int virDomainObjListSearchName(const void *payload,
- const void *name ATTRIBUTE_UNUSED,
- const void *data)
-{
- virDomainObjPtr obj = (virDomainObjPtr)payload;
- int want = 0;
-
- virObjectLock(obj);
- if (STREQ(obj->def->name, (const char *)data))
- want = 1;
- virObjectUnlock(obj);
- return want;
-}
-
virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
const char *name)
{
virDomainObjPtr obj;
+
virObjectLock(doms);
- obj = virHashSearch(doms->objs, virDomainObjListSearchName, name);
+ obj = virHashLookup(doms->objsName, name);
virObjectRef(obj);
+ virObjectUnlock(doms);
if (obj) {
virObjectLock(obj);
if (obj->removing) {
@@ -1165,7 +1159,6 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr
doms,
obj = NULL;
}
}
- virObjectUnlock(doms);
return obj;
}
@@ -2544,7 +2537,7 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
oldDef);
} else {
/* UUID does not match, but if a name matches, refuse it */
- if ((vm = virHashSearch(doms->objs, virDomainObjListSearchName,
def->name))) {
+ if ((vm = virHashLookup(doms->objsName, def->name))) {
virObjectLock(vm);
virUUIDFormat(vm->def->uuid, uuidstr);
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -2562,6 +2555,15 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
virObjectUnref(vm);
return NULL;
}
+
+ if (virHashAddEntry(doms->objsName, def->name, vm) < 0) {
+ virHashRemoveEntry(doms->objs, uuidstr);
+ return NULL;
+ }
+
+ /* Since domain is in two hash tables, increment the
+ * reference counter */
+ virObjectRef(vm);
}
cleanup:
return vm;
@@ -2719,6 +2721,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virObjectLock(doms);
virObjectLock(dom);
virHashRemoveEntry(doms->objs, uuidstr);
+ virHashRemoveEntry(doms->objsName, dom->def->name);
virObjectUnlock(dom);
virObjectUnref(dom);
virObjectUnlock(doms);
@@ -2736,9 +2739,10 @@ void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
char uuidstr[VIR_UUID_STRING_BUFLEN];
virUUIDFormat(dom->def->uuid, uuidstr);
- virObjectUnlock(dom);
virHashRemoveEntry(doms->objs, uuidstr);
+ virHashRemoveEntry(doms->objsName, dom->def->name);
+ virObjectUnlock(dom);
}
static int
@@ -21587,6 +21591,15 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms,
if (virHashAddEntry(doms->objs, uuidstr, obj) < 0)
goto error;
+ if (virHashAddEntry(doms->objsName, obj->def->name, obj) < 0) {
+ virHashRemoveEntry(doms->objs, uuidstr);
+ goto error;
+ }
+
+ /* Since domain is in two hash tables, increment the
+ * reference counter */
+ virObjectRef(obj);
+
if (notify)
(*notify)(obj, 1, opaque);
--
2.0.5
4:34 a.m.
New subject: [libvirt] [PATCH 5/5] virDomainObjList: Introduce yet another hash table
On Thu, Apr 23, 2015 at 19:14:58 +0200, Michal Privoznik wrote:
This hash table will contain the same data as already existing one.
The only difference is that while the first table uses domain uuid as
key, the new table uses domain name. This will allow much faster (and
lockless) lookups by domain name.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 51 +++++++++++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 19 deletions(-)
ACK,
Peter
10:11 p.m.
On 2015/4/24 1:14, Michal Privoznik wrote:
As discussed here:
https://www.redhat.com/archives/libvir-list/2015-April/msg01135.html
Michal Privoznik (5):
virDomainObjListAddLocked: s/false/NULL/ for @oldDef
virDomainObjListNew: Use virObjectFreeHashData
Introduce virDomainObjEndAPI
virDomainObjListFindByName: Return referenced object
virDomainObjList: Introduce yet another hash table
src/bhyve/bhyve_driver.c | 3 +-
src/conf/domain_conf.c | 83 +++++++++++++++++++++++------------
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/libxl/libxl_driver.c | 10 ++---
src/lxc/lxc_driver.c | 3 +-
src/openvz/openvz_driver.c | 11 +++--
src/parallels/parallels_driver.c | 3 +-
src/qemu/qemu_domain.c | 7 +--
src/qemu/qemu_driver.c | 14 ++----
src/test/test_driver.c | 93 +++++++++++++---------------------------
src/uml/uml_driver.c | 15 +++----
12 files changed, 110 insertions(+), 135 deletions(-)
Great. I tested the patch series, the migration downtime problem disappears.
12:40 a.m.
On 2015/4/24 1:14, Michal Privoznik wrote:
As discussed here:
https://www.redhat.com/archives/libvir-list/2015-April/msg01135.html
Michal Privoznik (5):
virDomainObjListAddLocked: s/false/NULL/ for @oldDef
virDomainObjListNew: Use virObjectFreeHashData
Introduce virDomainObjEndAPI
virDomainObjListFindByName: Return referenced object
virDomainObjList: Introduce yet another hash table
src/bhyve/bhyve_driver.c | 3 +-
src/conf/domain_conf.c | 83 +++++++++++++++++++++++------------
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/libxl/libxl_driver.c | 10 ++---
src/lxc/lxc_driver.c | 3 +-
src/openvz/openvz_driver.c | 11 +++--
src/parallels/parallels_driver.c | 3 +-
src/qemu/qemu_domain.c | 7 +--
src/qemu/qemu_driver.c | 14 ++----
src/test/test_driver.c | 93 +++++++++++++---------------------------
src/uml/uml_driver.c | 15 +++----
12 files changed, 110 insertions(+), 135 deletions(-)
BTW, we just dealt with virDomainObjListFindByName() here, without caring about
virDomainObjListFindByID().
virDomainObjListFindByID() is now called by umlDomainDestroyFlags() and
umlDomainShutdownFlags(),
if we simultaneously shutdown multiple vms on hypervisor UML, then similar problem comes
out:
some guest maybe unable to shutdown immediately, until other guests get shutoff, and even
'virsh list' blocks.
so, shall we:
1 don't take such problem as a problem
2 use virDomainObjListFindByName() instead of virDomainObjListFindByID() there.
3 add a third hash table for domid
If we use virDomainObjListFindByName() there, and forbids developers from using **ByID(),
it seems unacceptable for
any developers.
If we add a third hash table, it may become not easy to maintain the high-complexity
codes. Is there a better solution other
than adding more hash tables?
2:16 a.m.
On 24.04.2015 07:40, zhang bo wrote:
On 2015/4/24 1:14, Michal Privoznik wrote:
> As discussed here:
>
> https://www.redhat.com/archives/libvir-list/2015-April/msg01135.html
>
> Michal Privoznik (5):
> virDomainObjListAddLocked: s/false/NULL/ for @oldDef
> virDomainObjListNew: Use virObjectFreeHashData
> Introduce virDomainObjEndAPI
> virDomainObjListFindByName: Return referenced object
> virDomainObjList: Introduce yet another hash table
>
> src/bhyve/bhyve_driver.c | 3 +-
> src/conf/domain_conf.c | 83 +++++++++++++++++++++++------------
> src/conf/domain_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/libxl/libxl_driver.c | 10 ++---
> src/lxc/lxc_driver.c | 3 +-
> src/openvz/openvz_driver.c | 11 +++--
> src/parallels/parallels_driver.c | 3 +-
> src/qemu/qemu_domain.c | 7 +--
> src/qemu/qemu_driver.c | 14 ++----
> src/test/test_driver.c | 93 +++++++++++++---------------------------
> src/uml/uml_driver.c | 15 +++----
> 12 files changed, 110 insertions(+), 135 deletions(-)
>
BTW, we just dealt with virDomainObjListFindByName() here, without caring about
virDomainObjListFindByID().
virDomainObjListFindByID() is now called by umlDomainDestroyFlags() and
umlDomainShutdownFlags(),
if we simultaneously shutdown multiple vms on hypervisor UML, then similar problem comes
out:
some guest maybe unable to shutdown immediately, until other guests get shutoff, and even
'virsh list' blocks.
so, shall we:
1 don't take such problem as a problem
2 use virDomainObjListFindByName() instead of virDomainObjListFindByID() there.
In fact I'd prefer FindByUUID() everywhere except for those few places
where other lookup functions are necessary (e.g. virDomainLookupByID()).
3 add a third hash table for domid
No. even two hash tables duplicate information enough.
If we use virDomainObjListFindByName() there, and forbids developers from using **ByID(),
it seems unacceptable for
any developers.
Why?
If we add a third hash table, it may become not easy to maintain the
high-complexity codes. Is there a better solution other
than adding more hash tables?
The ultimate solution would be to have multi key hash table, so that any
item from tuple (uuid, name, id) would suffice to find domain object.
Michal
3:28 a.m.
On Fri, Apr 24, 2015 at 09:16:05AM +0200, Michal Privoznik wrote:
On 24.04.2015 07:40, zhang bo wrote:
> On 2015/4/24 1:14, Michal Privoznik wrote:
>
>> As discussed here:
>>
>> https://www.redhat.com/archives/libvir-list/2015-April/msg01135.html
>>
>> Michal Privoznik (5):
>> virDomainObjListAddLocked: s/false/NULL/ for @oldDef
>> virDomainObjListNew: Use virObjectFreeHashData
>> Introduce virDomainObjEndAPI
>> virDomainObjListFindByName: Return referenced object
>> virDomainObjList: Introduce yet another hash table
>>
>> src/bhyve/bhyve_driver.c | 3 +-
>> src/conf/domain_conf.c | 83 +++++++++++++++++++++++------------
>> src/conf/domain_conf.h | 2 +
>> src/libvirt_private.syms | 1 +
>> src/libxl/libxl_driver.c | 10 ++---
>> src/lxc/lxc_driver.c | 3 +-
>> src/openvz/openvz_driver.c | 11 +++--
>> src/parallels/parallels_driver.c | 3 +-
>> src/qemu/qemu_domain.c | 7 +--
>> src/qemu/qemu_driver.c | 14 ++----
>> src/test/test_driver.c | 93 +++++++++++++---------------------------
>> src/uml/uml_driver.c | 15 +++----
>> 12 files changed, 110 insertions(+), 135 deletions(-)
>>
>
>
> BTW, we just dealt with virDomainObjListFindByName() here, without caring about
virDomainObjListFindByID().
> virDomainObjListFindByID() is now called by umlDomainDestroyFlags() and
umlDomainShutdownFlags(),
> if we simultaneously shutdown multiple vms on hypervisor UML, then similar problem
comes out:
> some guest maybe unable to shutdown immediately, until other guests get shutoff, and
even 'virsh list' blocks.
>
> so, shall we:
> 1 don't take such problem as a problem
> 2 use virDomainObjListFindByName() instead of virDomainObjListFindByID() there.
In fact I'd prefer FindByUUID() everywhere except for those few places
where other lookup functions are necessary (e.g. virDomainLookupByID()).
> 3 add a third hash table for domid
No. even two hash tables duplicate information enough.
>
> If we use virDomainObjListFindByName() there, and forbids developers from using
**ByID(), it seems unacceptable for
> any developers.
Why?
> If we add a third hash table, it may become not easy to maintain the high-complexity
codes. Is there a better solution other
> than adding more hash tables?
>
The ultimate solution would be to have multi key hash table, so that any
item from tuple (uuid, name, id) would suffice to find domain object.
I think we should just ignore the FindByID method - nothing inside
libvirt should use it - as you say we should always use UUID except
in the couple of places we use name. The public LookupByID method
is discouraged now we have the bulk-list APIs. So I don't think
there's any compelling reason to have a 3rd hash for ID lookup.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
6:27 a.m.
On 23.04.2015 19:14, Michal Privoznik wrote:
As discussed here:
https://www.redhat.com/archives/libvir-list/2015-April/msg01135.html
Michal Privoznik (5):
virDomainObjListAddLocked: s/false/NULL/ for @oldDef
virDomainObjListNew: Use virObjectFreeHashData
Introduce virDomainObjEndAPI
virDomainObjListFindByName: Return referenced object
virDomainObjList: Introduce yet another hash table
src/bhyve/bhyve_driver.c | 3 +-
src/conf/domain_conf.c | 83 +++++++++++++++++++++++------------
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/libxl/libxl_driver.c | 10 ++---
src/lxc/lxc_driver.c | 3 +-
src/openvz/openvz_driver.c | 11 +++--
src/parallels/parallels_driver.c | 3 +-
src/qemu/qemu_domain.c | 7 +--
src/qemu/qemu_driver.c | 14 ++----
src/test/test_driver.c | 93 +++++++++++++---------------------------
src/uml/uml_driver.c | 15 +++----
12 files changed, 110 insertions(+), 135 deletions(-)
Thanks for all the reviews. I've pushed this.
Michal
3531
days inactive
3532
days old
15 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa -
zhang bo