Changeset
src/libxl/libxl_driver.c    | 40 ++++++++++++++++++++++------------------
src/libxl/libxl_migration.c | 10 ++--------
2 files changed, 24 insertions(+), 26 deletions(-)
Git apply log
Switched to a new branch '20180313172634.27237-1-jfehlig@suse.com'
Applying: libxl: MigrateBegin: Dont call EndAPI in helper function
Applying: libxl: MigrateConfirm: Dont unlock virDomainObj in helper function
Applying: libxl: MigratePrepare: properly cleanup after virDomainObjListAdd
Applying: libxl: MigratePerform: properly cleanup after libxlDomObjFromDomain
To https://github.com/patchew-project/libvirt
 + f461cbb...3edbce3 patchew/20180313172634.27237-1-jfehlig@suse.com -> patchew/20180313172634.27237-1-jfehlig@suse.com (forced update)
Test passed: syntax-check

loading

[libvirt] [PATCH 0/4] libxl: fix virDomainObj locking and ref counting in migration APIs
Posted by Jim Fehlig, 14 weeks ago
While reviewing a patch [0] from John's series to rework
virDomainObjListFindBy{UUID|ID}Ref, I noticed several problems with locking
and ref counting in the libxl migration APIs. This series changes the Begin,
Prepare, Perform, and Confirm APIs to use the standard pattern of get a
locked and ref counted virDomainObj, perform API, virDomainObjEndAPI.

[0] https://www.redhat.com/archives/libvir-list/2018-March/msg00516.html

Jim Fehlig (4):
  libxl: MigrateBegin: Dont call EndAPI in helper function
  libxl: MigrateConfirm: Dont unlock virDomainObj in helper function
  libxl: MigratePrepare: properly cleanup after virDomainObjListAdd
  libxl: MigratePerform: properly cleanup after libxlDomObjFromDomain

 src/libxl/libxl_driver.c    | 40 ++++++++++++++++++++++------------------
 src/libxl/libxl_migration.c | 10 ++--------
 2 files changed, 24 insertions(+), 26 deletions(-)

-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] libxl: fix virDomainObj locking and ref counting in migration APIs
Posted by Michal Privoznik, 14 weeks ago
On 03/13/2018 06:26 PM, Jim Fehlig wrote:
> While reviewing a patch [0] from John's series to rework
> virDomainObjListFindBy{UUID|ID}Ref, I noticed several problems with locking
> and ref counting in the libxl migration APIs. This series changes the Begin,
> Prepare, Perform, and Confirm APIs to use the standard pattern of get a
> locked and ref counted virDomainObj, perform API, virDomainObjEndAPI.
> 
> [0] https://www.redhat.com/archives/libvir-list/2018-March/msg00516.html
> 
> Jim Fehlig (4):
>   libxl: MigrateBegin: Dont call EndAPI in helper function
>   libxl: MigrateConfirm: Dont unlock virDomainObj in helper function
>   libxl: MigratePrepare: properly cleanup after virDomainObjListAdd
>   libxl: MigratePerform: properly cleanup after libxlDomObjFromDomain
> 
>  src/libxl/libxl_driver.c    | 40 ++++++++++++++++++++++------------------
>  src/libxl/libxl_migration.c | 10 ++--------
>  2 files changed, 24 insertions(+), 26 deletions(-)
> 

ACK series.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] libxl: MigrateBegin: Dont call EndAPI in helper function
Posted by Jim Fehlig, 14 weeks ago
The libxlDomainMigrateBegin3Params API locks and ref counts the associated
virDomainObj but relies on the helper function libxlDomainMigrationBegin
to unref/unlock the object. libxlDomainMigrationBegin is also used by
libxlDomainMigratePerform3Params for p2p migration, but in that case the
lock/ref and unref/unlock are properly handled in the API entry point. So
p2p migrations suffer a double unref/unlock in the Perform API.

Remove the unref/unlock (virDomainObjEndAPI) from libxlDomainMigrationBegin
and adjust libxlDomainMigrateBegin3Params to properly unref/unlock
the virDomainObj on success and error paths.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_driver.c    | 24 +++++++++++++-----------
 src/libxl/libxl_migration.c |  1 -
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index c3616a86d..eff13e5aa 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5876,6 +5876,7 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
 {
     const char *xmlin = NULL;
     virDomainObjPtr vm = NULL;
+    char *xmlout = NULL;
 
 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
     virReportUnsupportedError();
@@ -5895,25 +5896,26 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
         return NULL;
 
     if (STREQ_NULLABLE(vm->def->name, "Domain-0")) {
-            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-                           _("Domain-0 cannot be migrated"));
-            return NULL;
+        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                       _("Domain-0 cannot be migrated"));
+        goto cleanup;
     }
 
-    if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) {
-        virObjectUnlock(vm);
-        return NULL;
-    }
+    if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0)
+        goto cleanup;
 
     if (!virDomainObjIsActive(vm)) {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        "%s", _("domain is not running"));
-        virObjectUnlock(vm);
-        return NULL;
+        goto cleanup;
     }
 
-    return libxlDomainMigrationBegin(domain->conn, vm, xmlin,
-                                     cookieout, cookieoutlen);
+    xmlout = libxlDomainMigrationBegin(domain->conn, vm, xmlin,
+                                       cookieout, cookieoutlen);
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return xmlout;
 }
 
 static int
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index ccf2daed1..4b848c920 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -443,7 +443,6 @@ libxlDomainMigrationBegin(virConnectPtr conn,
 
  cleanup:
     libxlMigrationCookieFree(mig);
-    virDomainObjEndAPI(&vm);
     virDomainDefFree(tmpdef);
     virObjectUnref(cfg);
     return xml;
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] libxl: MigrateBegin: Dont call EndAPI in helper function
Posted by John Ferlan, 14 weeks ago

On 03/13/2018 01:26 PM, Jim Fehlig wrote:
> The libxlDomainMigrateBegin3Params API locks and ref counts the associated
> virDomainObj but relies on the helper function libxlDomainMigrationBegin
> to unref/unlock the object. libxlDomainMigrationBegin is also used by
> libxlDomainMigratePerform3Params for p2p migration, but in that case the
> lock/ref and unref/unlock are properly handled in the API entry point. So
> p2p migrations suffer a double unref/unlock in the Perform API.
> 
> Remove the unref/unlock (virDomainObjEndAPI) from libxlDomainMigrationBegin
> and adjust libxlDomainMigrateBegin3Params to properly unref/unlock
> the virDomainObj on success and error paths.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c    | 24 +++++++++++++-----------
>  src/libxl/libxl_migration.c |  1 -
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

BTW: Outside the scope of this series; however, danpb went through the
painstaking task of modifying names of qemu_migration API's such that
it's easier to determine if the API was run on qemuMigrationSrc or
qemuMigrationDst - made it so much easier to remember which was which
w/r/t the various stages - you may want to consider doing that too for
libxl!

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] libxl: MigrateBegin: Dont call EndAPI in helper function
Posted by Jim Fehlig, 14 weeks ago
On 03/14/2018 06:43 AM, John Ferlan wrote:
> 
> 
> On 03/13/2018 01:26 PM, Jim Fehlig wrote:
>> The libxlDomainMigrateBegin3Params API locks and ref counts the associated
>> virDomainObj but relies on the helper function libxlDomainMigrationBegin
>> to unref/unlock the object. libxlDomainMigrationBegin is also used by
>> libxlDomainMigratePerform3Params for p2p migration, but in that case the
>> lock/ref and unref/unlock are properly handled in the API entry point. So
>> p2p migrations suffer a double unref/unlock in the Perform API.
>>
>> Remove the unref/unlock (virDomainObjEndAPI) from libxlDomainMigrationBegin
>> and adjust libxlDomainMigrateBegin3Params to properly unref/unlock
>> the virDomainObj on success and error paths.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>   src/libxl/libxl_driver.c    | 24 +++++++++++++-----------
>>   src/libxl/libxl_migration.c |  1 -
>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>

Thanks for reviewing the series!

> BTW: Outside the scope of this series; however, danpb went through the
> painstaking task of modifying names of qemu_migration API's such that
> it's easier to determine if the API was run on qemuMigrationSrc or
> qemuMigrationDst - made it so much easier to remember which was which
> w/r/t the various stages - you may want to consider doing that too for
> libxl!

Good point. I'm familiar with the code and still find myself thinking about 
which phase pertains to the src and which to the dest.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] libxl: MigrateConfirm: Dont unlock virDomainObj in helper function
Posted by Jim Fehlig, 14 weeks ago
The libxlDomainMigrateConfirm3Params API locks and ref counts the associated
virDomainObj but relies on the helper function libxlDomainMigrationConfirm
to unlock the object. Unref'ing the object is not done in either function.
libxlDomainMigrationConfirm is also used by libxlDomainMigratePerform3Params
for p2p migration, but in that case the lock/ref and unref/unlock are
properly handled in the API entry point.

Remove the unlock from libxlDomainMigrationConfirm and adjust
libxlDomainMigrateConfirm3Params to properly unref/unlock the virDomainObj
on success and error paths.

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

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index eff13e5aa..67a638da0 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -6161,6 +6161,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
 {
     libxlDriverPrivatePtr driver = domain->conn->privateData;
     virDomainObjPtr vm = NULL;
+    int ret = -1;
 
 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
     virReportUnsupportedError();
@@ -6174,12 +6175,14 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
     if (!(vm = libxlDomObjFromDomain(domain)))
         return -1;
 
-    if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) {
-        virObjectUnlock(vm);
-        return -1;
-    }
+    if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0)
+        goto cleanup;
 
-    return libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
+    ret = libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return ret;
 }
 
 static int libxlNodeGetSecurityModel(virConnectPtr conn,
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 4b848c920..fef1c998b 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -1400,8 +1400,6 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver,
  cleanup:
     if (event)
         libxlDomainEventQueue(driver, event);
-    if (vm)
-        virObjectUnlock(vm);
     virObjectUnref(cfg);
     return ret;
 }
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] libxl: MigrateConfirm: Dont unlock virDomainObj in helper function
Posted by John Ferlan, 14 weeks ago

On 03/13/2018 01:26 PM, Jim Fehlig wrote:
> The libxlDomainMigrateConfirm3Params API locks and ref counts the associated
> virDomainObj but relies on the helper function libxlDomainMigrationConfirm
> to unlock the object. Unref'ing the object is not done in either function.
> libxlDomainMigrationConfirm is also used by libxlDomainMigratePerform3Params
> for p2p migration, but in that case the lock/ref and unref/unlock are
> properly handled in the API entry point.
> 
> Remove the unlock from libxlDomainMigrationConfirm and adjust
> libxlDomainMigrateConfirm3Params to properly unref/unlock the virDomainObj
> on success and error paths.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c    | 13 ++++++++-----
>  src/libxl/libxl_migration.c |  2 --
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index eff13e5aa..67a638da0 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -6161,6 +6161,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
>  {
>      libxlDriverPrivatePtr driver = domain->conn->privateData;
>      virDomainObjPtr vm = NULL;
> +    int ret = -1;
>  
>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>      virReportUnsupportedError();
> @@ -6174,12 +6175,14 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
>      if (!(vm = libxlDomObjFromDomain(domain)))
>          return -1;
>  
> -    if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) {
> -        virObjectUnlock(vm);
> -        return -1;
> -    }
> +    if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0)
> +        goto cleanup;
>  
> -    return libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
> +    ret = libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
> +
> + cleanup:
> +    virDomainObjEndAPI(&vm);
> +    return ret;
>  }
>  
>  static int libxlNodeGetSecurityModel(virConnectPtr conn,
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 4b848c920..fef1c998b 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -1400,8 +1400,6 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver,

Above here there is a possible call to virDomainObjListRemove which
would return @vm w/ at least 1 ref and unlocked. The reason it's "at
least 1 ref" is because some call paths in libxl will call
virDomainObjListAdd *and* then perform a virObjectRef(vm) while others
don't do that ref. Return from ListAdd will have 2 refs and 1 lock on
@vm (although it should be 3, hence my other series). It's not
necessarily crystal clear which ListAdd was used in this path...

In any case, "for now" I think we're mostly OK here because at least @vm
was obtained via libxlDomObjFromDomain which will return @vm w/ 1 extra
ref and locked (e.g. 3 refs and locked). So calling
virDomainObjListRemove will remove 2 refs and the lock.

All it means is the EndAPI call done will call Unlock even though it
doesn't have the lock. Of course there's no error propagation, so it
doesn't hurt. On the upside, the changes will at least allow @vm to be
free'd once the EndAPI is called (when @vm refcnt == 0) as opposed to
the changes before here where refcnt was never lowered and @vm was
probably leaked once/if it was removed from the list.

I think what could be done is - after calling ListRemove, you could call
virObjectLock(vm) and remove the vm = NULL.  That way you know you
"leave" this function with at least 1 ref and @vm being locked
regardless of what happens. That way the caller using EndAPI will do the
right thing too.

Hopefully this makes sense, I have various stages of this ListAdd
problem percolating inside my head...

Consider this:

Reviewed-by: John Ferlan <jferlan@redhat.com>

Hopefully you can look/think about the Remove/Lock and confirm my
thoughts...

John


>   cleanup:
>      if (event)
>          libxlDomainEventQueue(driver, event);
> -    if (vm)
> -        virObjectUnlock(vm);
>      virObjectUnref(cfg);
>      return ret;
>  }
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] libxl: MigrateConfirm: Dont unlock virDomainObj in helper function
Posted by Jim Fehlig, 14 weeks ago
On 03/14/2018 06:53 AM, John Ferlan wrote:
> 
> 
> On 03/13/2018 01:26 PM, Jim Fehlig wrote:
>> The libxlDomainMigrateConfirm3Params API locks and ref counts the associated
>> virDomainObj but relies on the helper function libxlDomainMigrationConfirm
>> to unlock the object. Unref'ing the object is not done in either function.
>> libxlDomainMigrationConfirm is also used by libxlDomainMigratePerform3Params
>> for p2p migration, but in that case the lock/ref and unref/unlock are
>> properly handled in the API entry point.
>>
>> Remove the unlock from libxlDomainMigrationConfirm and adjust
>> libxlDomainMigrateConfirm3Params to properly unref/unlock the virDomainObj
>> on success and error paths.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>   src/libxl/libxl_driver.c    | 13 ++++++++-----
>>   src/libxl/libxl_migration.c |  2 --
>>   2 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index eff13e5aa..67a638da0 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -6161,6 +6161,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
>>   {
>>       libxlDriverPrivatePtr driver = domain->conn->privateData;
>>       virDomainObjPtr vm = NULL;
>> +    int ret = -1;
>>   
>>   #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>>       virReportUnsupportedError();
>> @@ -6174,12 +6175,14 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
>>       if (!(vm = libxlDomObjFromDomain(domain)))
>>           return -1;
>>   
>> -    if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) {
>> -        virObjectUnlock(vm);
>> -        return -1;
>> -    }
>> +    if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0)
>> +        goto cleanup;
>>   
>> -    return libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
>> +    ret = libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
>> +
>> + cleanup:
>> +    virDomainObjEndAPI(&vm);
>> +    return ret;
>>   }
>>   
>>   static int libxlNodeGetSecurityModel(virConnectPtr conn,
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index 4b848c920..fef1c998b 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -1400,8 +1400,6 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver,
> 
> Above here there is a possible call to virDomainObjListRemove which
> would return @vm w/ at least 1 ref and unlocked. The reason it's "at
> least 1 ref" is because some call paths in libxl will call
> virDomainObjListAdd *and* then perform a virObjectRef(vm) while others
> don't do that ref. Return from ListAdd will have 2 refs and 1 lock on
> @vm (although it should be 3, hence my other series). It's not
> necessarily crystal clear which ListAdd was used in this path...
> 
> In any case, "for now" I think we're mostly OK here because at least @vm
> was obtained via libxlDomObjFromDomain which will return @vm w/ 1 extra
> ref and locked (e.g. 3 refs and locked). So calling
> virDomainObjListRemove will remove 2 refs and the lock.
> 
> All it means is the EndAPI call done will call Unlock even though it
> doesn't have the lock. Of course there's no error propagation, so it
> doesn't hurt. On the upside, the changes will at least allow @vm to be
> free'd once the EndAPI is called (when @vm refcnt == 0) as opposed to
> the changes before here where refcnt was never lowered and @vm was
> probably leaked once/if it was removed from the list.
> 
> I think what could be done is - after calling ListRemove, you could call
> virObjectLock(vm) and remove the vm = NULL.  That way you know you
> "leave" this function with at least 1 ref and @vm being locked
> regardless of what happens. That way the caller using EndAPI will do the
> right thing too.

Thanks for the details. I agree with your suggestion and made the changes before 
pushing, along with adding a comment. E.g.

     virDomainObjListRemove(driver->domains, vm);
     /* Caller passed a locked vm and expects the same on return */
     virObjectLock(vm);

> Hopefully this makes sense, I have various stages of this ListAdd
> problem percolating inside my head...

Yes it does, and has helped me get my head around locking and ref counting the 
virDomainObj. It's also encouraged me to audit the libxl code for other locking 
and ref counting bugs.

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] libxl: MigratePrepare: properly cleanup after virDomainObjListAdd
Posted by Jim Fehlig, 14 weeks ago
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
Re: [libvirt] [PATCH 3/4] libxl: MigratePrepare: properly cleanup after virDomainObjListAdd
Posted by John Ferlan, 14 weeks ago

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
Re: [libvirt] [PATCH 3/4] libxl: MigratePrepare: properly cleanup after virDomainObjListAdd
Posted by Jim Fehlig, 14 weeks ago
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
[libvirt] [PATCH 4/4] libxl: MigratePerform: properly cleanup after libxlDomObjFromDomain
Posted by Jim Fehlig, 14 weeks ago
ibxlDomObjFromDomain to returns locked and ref counted virDomainObj but
libxlDomainMigratePerform3Params only unlocks the object on exit. Convert
it to use the virDomainObjEndAPI function for cleanup.

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

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 67a638da0..6f1c69b1d 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -6086,8 +6086,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] libxl: MigratePerform: properly cleanup after libxlDomObjFromDomain
Posted by John Ferlan, 14 weeks ago

On 03/13/2018 01:26 PM, Jim Fehlig wrote:
> ibxlDomObjFromDomain to returns locked and ref counted virDomainObj but

libxlDom...

> libxlDomainMigratePerform3Params only unlocks the object on exit. Convert
> it to use the virDomainObjEndAPI function for cleanup.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

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