[libvirt] [PATCH 0/5] Make virDomainObjListFindByName run in O(1)
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
It's a pointer after all. We should initialize it to NULL instead of false. Signed-off-by: Michal Privoznik <mprivozn@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
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@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK, Peter
There's no point in duplicating virObjectFreeHashData() in a separate function. Signed-off-by: Michal Privoznik <mprivozn@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
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@redhat.com> --- src/conf/domain_conf.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
ACK, Peter
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@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
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@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
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@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
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@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
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@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
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@redhat.com> --- src/conf/domain_conf.c | 51 +++++++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 19 deletions(-)
ACK, Peter
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.
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?
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
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 :|
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
participants (4)
-
Daniel P. Berrange -
Michal Privoznik -
Peter Krempa -
zhang bo