[libvirt] [PATCH 0/3] vir*ObjListAddLocked(): Produce better error message than 'Duplicate key'
*** BLURB HERE *** Michal Prívozník (3): DO NOT APPLY: Simple reproducer for virDomainObjListRemove virDomainObjListAddLocked: Produce better error message than 'Duplicate key' virNWFilterBindingObjListAddLocked: Produce better error message than 'Duplicate key' src/conf/virdomainobjlist.c | 36 +++++++++++++++++----------- src/conf/virnwfilterbindingobjlist.c | 29 +++++++++++++--------- 2 files changed, 40 insertions(+), 25 deletions(-) -- 2.19.2
This bug demonstrates itself for other objects too, but let's show it for domains. Assume that dummy.xml contains a valid domain definition. Then 1) virsh create dummy.xml && sleep 1 2) virsh destroy dummy & 3) virsh create ../dummy.xml Observe error: error: Failed to create domain from dummy.xml error: internal error: Duplicate key Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virdomainobjlist.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index a814fc10a3..3631cf16d0 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -396,6 +396,7 @@ virDomainObjListRemove(virDomainObjListPtr doms, dom->removing = true; virObjectRef(dom); virObjectUnlock(dom); + sleep(60); virObjectRWLockWrite(doms); virObjectLock(dom); virDomainObjListRemoveLocked(doms, dom); -- 2.19.2
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> --- 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 3631cf16d0..23734ad815 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -141,14 +141,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; } @@ -172,6 +167,12 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms, obj = virDomainObjListFindByUUIDLocked(doms, uuid); virObjectRWUnlock(doms); + if (obj && obj->removing) { + virObjectUnlock(obj); + virObjectUnref(obj); + obj = NULL; + } + return obj; } @@ -183,14 +184,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; } @@ -214,6 +210,12 @@ virDomainObjListFindByName(virDomainObjListPtr doms, obj = virDomainObjListFindByNameLocked(doms, name); virObjectRWUnlock(doms); + if (obj && obj->removing) { + virObjectUnlock(obj); + virObjectUnref(obj); + obj = NULL; + } + return obj; } @@ -285,8 +287,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.19.2
On 3/19/19 9:49 AM, Michal Privoznik wrote:
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> --- 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 3631cf16d0..23734ad815 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -141,14 +141,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; } @@ -172,6 +167,12 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms, obj = virDomainObjListFindByUUIDLocked(doms, uuid); virObjectRWUnlock(doms);
+ if (obj && obj->removing) { + virObjectUnlock(obj); + virObjectUnref(obj); + obj = NULL; + } + return obj; }
@@ -183,14 +184,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; } @@ -214,6 +210,12 @@ virDomainObjListFindByName(virDomainObjListPtr doms, obj = virDomainObjListFindByNameLocked(doms, name); virObjectRWUnlock(doms);
+ if (obj && obj->removing) { + virObjectUnlock(obj); + virObjectUnref(obj); + obj = NULL; + } + return obj; }
@@ -285,8 +287,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)) {
Extra space here
+ /* 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"),
Makes sense to me. virDomainObjListFindByID more or less already does the same thing. I thought about the locking a lot and couldn't come up with a reason why this would be problematic Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
If there are two concurrent threads, one of which is removing an nwfilter from the list and the other is trying to add it back they may serialize in the following order: 1) obj->removing is set and @obj is unlocked. 2) The tread that's trying to add the nwfilter onto the list locks the list and tries to find, if the nwfilter 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 function (virNWFilterBindingObjListFindByPortDevLocked()) 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 API. The fix consists of moving 'removing' check one level up and thus allowing virNWFilterBindingObjListAddLocked() to produce meaningful error message. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virnwfilterbindingobjlist.c | 29 +++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/conf/virnwfilterbindingobjlist.c b/src/conf/virnwfilterbindingobjlist.c index 06ccbf53af..dbeee0d55e 100644 --- a/src/conf/virnwfilterbindingobjlist.c +++ b/src/conf/virnwfilterbindingobjlist.c @@ -91,14 +91,9 @@ virNWFilterBindingObjListFindByPortDevLocked(virNWFilterBindingObjListPtr bindin virNWFilterBindingObjPtr obj; obj = virHashLookup(bindings->objs, name); - virObjectRef(obj); if (obj) { + virObjectRef(obj); virObjectLock(obj); - if (virNWFilterBindingObjGetRemoving(obj)) { - virObjectUnlock(obj); - virObjectUnref(obj); - obj = NULL; - } } return obj; } @@ -122,6 +117,12 @@ virNWFilterBindingObjListFindByPortDev(virNWFilterBindingObjListPtr bindings, obj = virNWFilterBindingObjListFindByPortDevLocked(bindings, name); virObjectRWUnlock(bindings); + if (obj && virNWFilterBindingObjGetRemoving(obj)) { + virObjectUnlock(obj); + virObjectUnref(obj); + obj = NULL; + } + return obj; } @@ -169,11 +170,17 @@ virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr bindings, virNWFilterBindingObjPtr binding; /* See if a binding with matching portdev already exists */ - if ((binding = virNWFilterBindingObjListFindByPortDevLocked( - bindings, def->portdevname))) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("binding '%s' already exists"), - def->portdevname); + binding = virNWFilterBindingObjListFindByPortDevLocked(bindings, def->portdevname); + if (binding) { + if (virNWFilterBindingObjGetRemoving(binding)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("binding '%s' is already being removed"), + def->portdevname); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("binding '%s' already exists"), + def->portdevname); + } goto error; } -- 2.19.2
On 3/19/19 9:49 AM, Michal Privoznik wrote:
If there are two concurrent threads, one of which is removing an nwfilter from the list and the other is trying to add it back they may serialize in the following order:
1) obj->removing is set and @obj is unlocked. 2) The tread that's trying to add the nwfilter onto the list locks the list and tries to find, if the nwfilter 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 function (virNWFilterBindingObjListFindByPortDevLocked()) 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 API. The fix consists of moving 'removing' check one level up and thus allowing virNWFilterBindingObjListAddLocked() to produce meaningful error message.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
On 3/19/19 2:49 PM, Michal Privoznik wrote:
*** BLURB HERE ***
Michal Prívozník (3): DO NOT APPLY: Simple reproducer for virDomainObjListRemove virDomainObjListAddLocked: Produce better error message than 'Duplicate key' virNWFilterBindingObjListAddLocked: Produce better error message than 'Duplicate key'
src/conf/virdomainobjlist.c | 36 +++++++++++++++++----------- src/conf/virnwfilterbindingobjlist.c | 29 +++++++++++++--------- 2 files changed, 40 insertions(+), 25 deletions(-)
ping? Michal
participants (2)
-
Cole Robinson -
Michal Privoznik