When one domain is being undefined and at the same time started, for example, there is a possibility of a rare problem occuring. - Thread 1 does virDomainUndefine(), has the lock, checks that the domain is active and because it's not, calls virDomainObjListRemove(). - Thread 2 does virDomainCreate() and tries to lock the domain. - Thread 1 needs to lock domain list in order to remove the domain from it, but must unlock domain first (proper order is to lock domain list first and the domain itself second). - Thread 2 grabs the lock, starts the domain and releases the lock. - Thread 1 grabs the lock and removes the domain from list. With this patch: - The undefining domain gets marked as "to undefine" before it is unlocked. - If domain is found in any of the search APIs, it's returned only if it is not marked as "to undefine". The check is done while the domain is locked. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1150505 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Notes: I marked this as v3, but it is basically a resend of v1 [1] with s/undefining/removing/ change. I'm sending this even though the second version of the patch got in again because the v2 that we pushed didn't help. I got convinced [2] that doing this with a job fixes it, but it doesn't, it just serializes the Create API after the Undefine API. The only difference that the commit b629c64e [3] does is that it waits on obtaining job, but not on locking the domain. That previous commit can be reverted if anyone wants, I see almost no point in it being there now. [1] http://www.redhat.com/archives/libvir-list/2014-October/msg01055.html [2] http://www.redhat.com/archives/libvir-list/2014-October/msg01115.html [3] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=b629c64e src/conf/domain_conf.c | 22 +++++++++++++++++++--- src/conf/domain_conf.h | 1 + 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ec45b8c..8e08407 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1062,8 +1062,13 @@ virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms, virDomainObjPtr obj; virObjectLock(doms); obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id); - if (obj) + if (obj) { virObjectLock(obj); + if (obj->removing) { + virObjectUnlock(obj); + obj = NULL; + } + } virObjectUnlock(doms); return obj; } @@ -1079,8 +1084,13 @@ virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms, virUUIDFormat(uuid, uuidstr); obj = virHashLookup(doms->objs, uuidstr); - if (obj) + if (obj) { virObjectLock(obj); + if (obj->removing) { + virObjectUnlock(obj); + obj = NULL; + } + } virObjectUnlock(doms); return obj; } @@ -1105,8 +1115,13 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, virDomainObjPtr obj; virObjectLock(doms); obj = virHashSearch(doms->objs, virDomainObjListSearchName, name); - if (obj) + if (obj) { virObjectLock(obj); + if (obj->removing) { + virObjectUnlock(obj); + obj = NULL; + } + } virObjectUnlock(doms); return obj; } @@ -2551,6 +2566,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms, { char uuidstr[VIR_UUID_STRING_BUFLEN]; + dom->removing = true; virUUIDFormat(dom->def->uuid, uuidstr); virObjectRef(dom); virObjectUnlock(dom); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e10b3c5..75e3d2a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2187,6 +2187,7 @@ struct _virDomainObj { unsigned int autostart : 1; unsigned int persistent : 1; unsigned int updated : 1; + unsigned int removing : 1; virDomainDefPtr def; /* The current definition */ virDomainDefPtr newDef; /* New definition to activate at shutdown */ -- 2.2.0