Use the FindBy{UUID|Name}Locked helpers which will return a locked
and ref counted object rather than the direct virHashLookup and
virObjectLock of the returned object. We'll need to temporarily
virObjectUnref when we assign a new domain @def, but that will
change shortly when virDomainObjListAddObjLocked returns the
correct reference counted object.

Use the virDomainObjEndAPI in the error path to Unref/Unlock for
the corresponding Unref/Unlock of either the FindBy* return or
the virDomainObjNew since both return a reffed/locked object.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/conf/virdomainobjlist.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 9aa2abd8c3..6752f6c572 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
     if (oldDef)
         *oldDef = NULL;
 
-    virUUIDFormat(def->uuid, uuidstr);
-
     /* See if a VM with matching UUID already exists */
-    if ((vm = virHashLookup(doms->objs, uuidstr))) {
-        virObjectLock(vm);
+    if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
         /* UUID matches, but if names don't match, refuse it */
         if (STRNEQ(vm->def->name, def->name)) {
             virUUIDFormat(vm->def->uuid, uuidstr);
@@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
                               def,
                               !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
                               oldDef);
+        /* XXX: Temporary until this API is fixed to return a locked and
+         *      refcnt'd object */
+        virObjectUnref(vm);
     } else {
         /* UUID does not match, but if a name matches, refuse it */
-        if ((vm = virHashLookup(doms->objsName, def->name))) {
-            virObjectLock(vm);
+        if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) {
             virUUIDFormat(vm->def->uuid, uuidstr);
             virReportError(VIR_ERR_OPERATION_FAILED,
                            _("domain '%s' already exists with uuid %s"),
@@ -329,18 +328,15 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
             goto cleanup;
         vm->def = def;
 
-        if (virDomainObjListAddObjLocked(doms, vm) < 0) {
-            virDomainObjEndAPI(&vm);
-            return NULL;
-        }
+        if (virDomainObjListAddObjLocked(doms, vm) < 0)
+            goto error;
     }
  cleanup:
     return vm;
 
  error:
-    virObjectUnlock(vm);
-    vm = NULL;
-    goto cleanup;
+    virDomainObjEndAPI(&vm);
+    return NULL;
 }
 
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 24, 2018 at 08:28:05AM -0400, John Ferlan wrote:
> Use the FindBy{UUID|Name}Locked helpers which will return a locked
> and ref counted object rather than the direct virHashLookup and
> virObjectLock of the returned object. We'll need to temporarily
> virObjectUnref when we assign a new domain @def, but that will
> change shortly when virDomainObjListAddObjLocked returns the
> correct reference counted object.
>
> Use the virDomainObjEndAPI in the error path to Unref/Unlock for
> the corresponding Unref/Unlock of either the FindBy* return or
> the virDomainObjNew since both return a reffed/locked object.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virdomainobjlist.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 9aa2abd8c3..6752f6c572 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>      if (oldDef)
>          *oldDef = NULL;
>
> -    virUUIDFormat(def->uuid, uuidstr);
> -
>      /* See if a VM with matching UUID already exists */
> -    if ((vm = virHashLookup(doms->objs, uuidstr))) {
> -        virObjectLock(vm);
> +    if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
>          /* UUID matches, but if names don't match, refuse it */
>          if (STRNEQ(vm->def->name, def->name)) {
>              virUUIDFormat(vm->def->uuid, uuidstr);
> @@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>                                def,
>                                !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
>                                oldDef);
> +        /* XXX: Temporary until this API is fixed to return a locked and
> +         *      refcnt'd object */
> +        virObjectUnref(vm);

I didn't bother trying, but would the tests pass even without ^this temporary
unref? Because if they do, we should drop it, since you do that anyway within
the same series, so who cares...

With or without the adjustment
Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 05/03/2018 12:02 PM, Erik Skultety wrote:
> On Tue, Apr 24, 2018 at 08:28:05AM -0400, John Ferlan wrote:
>> Use the FindBy{UUID|Name}Locked helpers which will return a locked
>> and ref counted object rather than the direct virHashLookup and
>> virObjectLock of the returned object. We'll need to temporarily
>> virObjectUnref when we assign a new domain @def, but that will
>> change shortly when virDomainObjListAddObjLocked returns the
>> correct reference counted object.
>>
>> Use the virDomainObjEndAPI in the error path to Unref/Unlock for
>> the corresponding Unref/Unlock of either the FindBy* return or
>> the virDomainObjNew since both return a reffed/locked object.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virdomainobjlist.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>> index 9aa2abd8c3..6752f6c572 100644
>> --- a/src/conf/virdomainobjlist.c
>> +++ b/src/conf/virdomainobjlist.c
>> @@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>>      if (oldDef)
>>          *oldDef = NULL;
>>
>> -    virUUIDFormat(def->uuid, uuidstr);
>> -
>>      /* See if a VM with matching UUID already exists */
>> -    if ((vm = virHashLookup(doms->objs, uuidstr))) {
>> -        virObjectLock(vm);
>> +    if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
>>          /* UUID matches, but if names don't match, refuse it */
>>          if (STRNEQ(vm->def->name, def->name)) {
>>              virUUIDFormat(vm->def->uuid, uuidstr);
>> @@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>>                                def,
>>                                !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
>>                                oldDef);
>> +        /* XXX: Temporary until this API is fixed to return a locked and
>> +         *      refcnt'd object */
>> +        virObjectUnref(vm);
> 
> I didn't bother trying, but would the tests pass even without ^this temporary
> unref? Because if they do, we should drop it, since you do that anyway within
> the same series, so who cares...
> 
> With or without the adjustment
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 

Yes, the tests would pass.

It's needed temporarily because virDomainObjListFindByUUIDLocked returns
a reffed/locked @vm; whereas, virHashLookup just returns @vm which was
locked before return.

The caller expects to receive @vm without that extra ref.  All things
being equal removing this would temporarily leak @vm. I was trying to be
"correct" through each stage of the changes.

And yes by patch 6 (or 4 if things are rearrange) it's a moot point
because the Add API will return the correct number of ref's on @vm.

Tks -

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list