Re: [libvirt] [PATCH 2/4] libxl: MigrateConfirm: Dont unlock virDomainObj in helper function

2018-03-16 Thread Jim Fehlig 

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 
---
  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();
+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

Re: [libvirt] [PATCH 2/4] libxl: MigrateConfirm: Dont unlock virDomainObj in helper function

2018-03-15 Thread John Ferlan 


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 
> ---
>  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();
> +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 

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

[libvirt] [PATCH 2/4] libxl: MigrateConfirm: Dont unlock virDomainObj in helper function

2018-03-13 Thread Jim Fehlig 
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 
---
 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();
+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