If the virHashAddEntry fails, then we need to "careful" about
how we free the @vm. When virDomainObjNew returns there is one
reference and the object is locked, so use virDomainObjEndAPI
when done.

Add a virObjectRef in the error path for the second virHashAddEntry
call since it doesn't call virObjectRef, but virHashRemoveEntry
will call virObjectUnref because virObjectFreeHashData is called
when the element is removed from the hash table as set up in
virDomainObjListNew.

Eventually these paths should goto error and error should be changed
to use EndAPI as well, but that requires more adjustments to other
paths in the code to have a locked and ref counted @vm.

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

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 87a742b1e..2ae8a41a1 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -296,12 +296,14 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
 
         virUUIDFormat(def->uuid, uuidstr);
         if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) {
-            virObjectUnref(vm);
+            virObjectEndAPI(&vm);
             return NULL;
         }
 
         if (virHashAddEntry(doms->objsName, def->name, vm) < 0) {
+            virObjectRef(vm);
             virHashRemoveEntry(doms->objs, uuidstr);
+            virObjectEndAPI(&vm);
             return NULL;
         }
 
-- 
2.13.6

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

On 03/29/2018 02:34 PM, John Ferlan wrote:
> If the virHashAddEntry fails, then we need to "careful" about
> how we free the @vm. When virDomainObjNew returns there is one
> reference and the object is locked, so use virDomainObjEndAPI
> when done.
> 
> Add a virObjectRef in the error path for the second virHashAddEntry
> call since it doesn't call virObjectRef, but virHashRemoveEntry
> will call virObjectUnref because virObjectFreeHashData is called
> when the element is removed from the hash table as set up in
> virDomainObjListNew.
> 
> Eventually these paths should goto error and error should be changed
> to use EndAPI as well, but that requires more adjustments to other
> paths in the code to have a locked and ref counted @vm.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/conf/virdomainobjlist.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 87a742b1e..2ae8a41a1 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -296,12 +296,14 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>  
>          virUUIDFormat(def->uuid, uuidstr);
>          if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) {
> -            virObjectUnref(vm);
> +            virObjectEndAPI(&vm);

s/virObjectEndAPI/virDomainObjEndAPI/ here and down too ;-)

>              return NULL;
>          }
>  
>          if (virHashAddEntry(doms->objsName, def->name, vm) < 0) {
> +            virObjectRef(vm);
>              virHashRemoveEntry(doms->objs, uuidstr);
> +            virObjectEndAPI(&vm);
>              return NULL;
>          }
>  
> 

ACK

Michal

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

On 04/06/2018 08:56 AM, Michal Privoznik wrote:
> On 03/29/2018 02:34 PM, John Ferlan wrote:
>> If the virHashAddEntry fails, then we need to "careful" about
>> how we free the @vm. When virDomainObjNew returns there is one
>> reference and the object is locked, so use virDomainObjEndAPI
>> when done.
>>
>> Add a virObjectRef in the error path for the second virHashAddEntry
>> call since it doesn't call virObjectRef, but virHashRemoveEntry
>> will call virObjectUnref because virObjectFreeHashData is called
>> when the element is removed from the hash table as set up in
>> virDomainObjListNew.
>>
>> Eventually these paths should goto error and error should be changed
>> to use EndAPI as well, but that requires more adjustments to other
>> paths in the code to have a locked and ref counted @vm.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>  src/conf/virdomainobjlist.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>> index 87a742b1e..2ae8a41a1 100644
>> --- a/src/conf/virdomainobjlist.c
>> +++ b/src/conf/virdomainobjlist.c
>> @@ -296,12 +296,14 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>>  
>>          virUUIDFormat(def->uuid, uuidstr);
>>          if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) {
>> -            virObjectUnref(vm);
>> +            virObjectEndAPI(&vm);
> 
> s/virObjectEndAPI/virDomainObjEndAPI/ here and down too ;-)
> 

Oh right - good catch... obviously I worked backwards from patch 3 but
my fingers mistyped things

Fixed...

Tks,

John

>>              return NULL;
>>          }
>>  
>>          if (virHashAddEntry(doms->objsName, def->name, vm) < 0) {
>> +            virObjectRef(vm);
>>              virHashRemoveEntry(doms->objs, uuidstr);
>> +            virObjectEndAPI(&vm);
>>              return NULL;
>>          }
>>  
>>
> 
> ACK
> 
> Michal
> 

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