124 lines
4.3 KiB
Diff
124 lines
4.3 KiB
Diff
From 874617d278b58554feee567b2e79d208aaef214c Mon Sep 17 00:00:00 2001
|
|
Message-Id: <874617d278b58554feee567b2e79d208aaef214c@dist-git>
|
|
From: Michal Privoznik <mprivozn@redhat.com>
|
|
Date: Tue, 6 Aug 2019 13:29:22 +0200
|
|
Subject: [PATCH] virDomainObjListAddLocked: Produce better error message than
|
|
'Duplicate key'
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
If there are two concurrent threads, one of which is removing a
|
|
domain from the list and the other is trying to add it back they
|
|
may serialize in the following order:
|
|
|
|
1) vm->removing is set and @vm is unlocked.
|
|
2) The tread that's trying to add the domain onto the list locks
|
|
the list and tries to find, if the domain already exists.
|
|
3) Our lookup functions say it doesn't, so the thread proceeds to
|
|
virHashAddEntry() which fails with 'Duplicate key' error.
|
|
|
|
This is obviously not helpful error message at all.
|
|
|
|
The problem lies in our lookup functions
|
|
(virDomainObjListFindByUUIDLocked() and
|
|
virDomainObjListFindByNameLocked()) which return NULL even if the
|
|
object is still on the list. They do this so that the object is
|
|
not mistakenly looked up by some driver. The fix consists of
|
|
moving 'removing' check one level up and thus allowing
|
|
virDomainObjListAddLocked() to produce meaningful error message.
|
|
|
|
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
|
|
Reviewed-by: Cole Robinson <crobinso@redhat.com>
|
|
(cherry picked from commit a5c71129bf2c12a827f1bc00149acd1c572ffe9c)
|
|
|
|
Signed-off-by: Ján Tomko <jtomko@redhat.com>
|
|
RHEL-8.1.0: https://bugzilla.redhat.com/show_bug.cgi?id=1737790
|
|
Message-Id: <c63ca487bf56af5240e273b5a112a753c2bc85a3.1565090956.git.jtomko@redhat.com>
|
|
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
|
|
---
|
|
src/conf/virdomainobjlist.c | 35 +++++++++++++++++++++--------------
|
|
1 file changed, 21 insertions(+), 14 deletions(-)
|
|
|
|
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
|
|
index e7c3e326ca..3b388e2ba2 100644
|
|
--- a/src/conf/virdomainobjlist.c
|
|
+++ b/src/conf/virdomainobjlist.c
|
|
@@ -142,14 +142,9 @@ virDomainObjListFindByUUIDLocked(virDomainObjListPtr doms,
|
|
|
|
virUUIDFormat(uuid, uuidstr);
|
|
obj = virHashLookup(doms->objs, uuidstr);
|
|
- virObjectRef(obj);
|
|
if (obj) {
|
|
+ virObjectRef(obj);
|
|
virObjectLock(obj);
|
|
- if (obj->removing) {
|
|
- virObjectUnlock(obj);
|
|
- virObjectUnref(obj);
|
|
- obj = NULL;
|
|
- }
|
|
}
|
|
return obj;
|
|
}
|
|
@@ -173,6 +168,12 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms,
|
|
obj = virDomainObjListFindByUUIDLocked(doms, uuid);
|
|
virObjectRWUnlock(doms);
|
|
|
|
+ if (obj && obj->removing) {
|
|
+ virObjectUnlock(obj);
|
|
+ virObjectUnref(obj);
|
|
+ obj = NULL;
|
|
+ }
|
|
+
|
|
return obj;
|
|
}
|
|
|
|
@@ -184,14 +185,9 @@ virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
|
|
virDomainObjPtr obj;
|
|
|
|
obj = virHashLookup(doms->objsName, name);
|
|
- virObjectRef(obj);
|
|
if (obj) {
|
|
+ virObjectRef(obj);
|
|
virObjectLock(obj);
|
|
- if (obj->removing) {
|
|
- virObjectUnlock(obj);
|
|
- virObjectUnref(obj);
|
|
- obj = NULL;
|
|
- }
|
|
}
|
|
return obj;
|
|
}
|
|
@@ -215,6 +211,12 @@ virDomainObjListFindByName(virDomainObjListPtr doms,
|
|
obj = virDomainObjListFindByNameLocked(doms, name);
|
|
virObjectRWUnlock(doms);
|
|
|
|
+ if (obj && obj->removing) {
|
|
+ virObjectUnlock(obj);
|
|
+ virObjectUnref(obj);
|
|
+ obj = NULL;
|
|
+ }
|
|
+
|
|
return obj;
|
|
}
|
|
|
|
@@ -286,8 +288,13 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
|
|
|
|
/* See if a VM with matching UUID already exists */
|
|
if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
|
|
- /* UUID matches, but if names don't match, refuse it */
|
|
- if (STRNEQ(vm->def->name, def->name)) {
|
|
+ if (vm->removing) {
|
|
+ virReportError(VIR_ERR_OPERATION_FAILED,
|
|
+ _("domain '%s' is already being removed"),
|
|
+ vm->def->name);
|
|
+ goto error;
|
|
+ } else if (STRNEQ(vm->def->name, def->name)) {
|
|
+ /* UUID matches, but if names don't match, refuse it */
|
|
virUUIDFormat(vm->def->uuid, uuidstr);
|
|
virReportError(VIR_ERR_OPERATION_FAILED,
|
|
_("domain '%s' is already defined with uuid %s"),
|
|
--
|
|
2.22.1
|
|
|