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