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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
