libxlDomainMigrationPrepare adds the incoming domain def to the list of
domains via virDomainObjListAdd, which returns a locked and ref counted
virDomainObj. On exit the object is unlocked but not unref'ed. The same
is true for libxlDomainMigrationPrepareTunnel3. Convert both to use the
virDomainObjEndAPI function for cleanup.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_migration.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index fef1c998b..6b53f9587 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -639,9 +639,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
     }
 
  done:
-    if (vm)
-        virObjectUnlock(vm);
-
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -820,8 +818,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
         VIR_FREE(hostname);
     else
         virURIFree(uri);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
-- 
2.16.2

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

On 03/13/2018 01:26 PM, Jim Fehlig wrote:
> libxlDomainMigrationPrepare adds the incoming domain def to the list of
> domains via virDomainObjListAdd, which returns a locked and ref counted
> virDomainObj. On exit the object is unlocked but not unref'ed. The same
> is true for libxlDomainMigrationPrepareTunnel3. Convert both to use the
> virDomainObjEndAPI function for cleanup.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_migration.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 

These two leave me concerned - mainly because as I described in my
review of 2/4 the ListAdd function will return 2 refs and 1 lock on the
object unlike the libxlDomObjFromDomain where there are at least 3 refs
and 1 lock. Since neither of these code paths adds a Ref(vm) after the
ListAdd call (like CreateXML and RestoreFlags do) that means calling
EndAPI will remove 1 ref and the lock leaving only 1 ref on the object.

Later on when ListRemove is called it would enter with 2 refs and 1 lock
and leave with @vm being destroyed. Not having a clear picture of all
the paths a @vm could have in libxl makes me concerned because there are
a few places where ListRemove is called *and* @vm is referenced
afterwards such as libxlDomainDestroyFlags

I think once ListAdd returns with the right number of refs, then yes,
this is the proper adjustment, but for now unless there's an extra Ref
done after the ListAdd, then we should leave things as is.

Thoughts? And does this make sense?


John

> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index fef1c998b..6b53f9587 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -639,9 +639,7 @@ libxlDomainMigrationPrepareTunnel3(virConnectPtr dconn,
>      }
>  
>   done:
> -    if (vm)
> -        virObjectUnlock(vm);
> -
> +    virDomainObjEndAPI(&vm);
>      return ret;
>  }
>  
> @@ -820,8 +818,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>          VIR_FREE(hostname);
>      else
>          virURIFree(uri);
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>      virObjectUnref(cfg);
>      return ret;
>  }
> 

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

On 03/14/2018 07:19 AM, John Ferlan wrote:
> 
> 
> On 03/13/2018 01:26 PM, Jim Fehlig wrote:
>> libxlDomainMigrationPrepare adds the incoming domain def to the list of
>> domains via virDomainObjListAdd, which returns a locked and ref counted
>> virDomainObj. On exit the object is unlocked but not unref'ed. The same
>> is true for libxlDomainMigrationPrepareTunnel3. Convert both to use the
>> virDomainObjEndAPI function for cleanup.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>   src/libxl/libxl_migration.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
> 
> These two leave me concerned - mainly because as I described in my
> review of 2/4 the ListAdd function will return 2 refs and 1 lock on the
> object unlike the libxlDomObjFromDomain where there are at least 3 refs
> and 1 lock. Since neither of these code paths adds a Ref(vm) after the
> ListAdd call (like CreateXML and RestoreFlags do) that means calling
> EndAPI will remove 1 ref and the lock leaving only 1 ref on the object.

I'll send a V2 of this patch that adds a ref after ListAdd so the pattern is 
consistent throughout the driver.

> Later on when ListRemove is called it would enter with 2 refs and 1 lock
> and leave with @vm being destroyed. Not having a clear picture of all
> the paths a @vm could have in libxl makes me concerned because there are
> a few places where ListRemove is called *and* @vm is referenced
> afterwards such as libxlDomainDestroyFlags
> 
> I think once ListAdd returns with the right number of refs, then yes,
> this is the proper adjustment, but for now unless there's an extra Ref
> done after the ListAdd, then we should leave things as is.
> 
> Thoughts? And does this make sense?

Yes, it makes sense. Thanks again for the details!

Regards,
Jim

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