1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 00/20] Rework/rename virDomainObjListFindBy{UUID|ID}Ref
  4. View patch

[libvirt] [PATCH 03/20] libxl: Properly cleanup after libxlDomObjFromDomain

John Ferlan posted 20 patches 7 years, 2 months ago
[libvirt] [PATCH 03/20] libxl: Properly cleanup after libxlDomObjFromDomain
Posted by John Ferlan 7 years, 2 months ago 
Commit id '9ac945078' altered libxlDomObjFromDomain to return
a locked *and* ref counted object for some specific purposes;
however, it neglected to alter all the consumers of the helper
to use virDomainObjEndAPI thus leaving many objects with extra
ref counts.

The two consumers for libxlDomainMigrationConfirm would also
originally use the libxlDomObjFromDomain API (either from
libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via
libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/libxl/libxl_driver.c    | 86 ++++++++++++++++-----------------------------
 src/libxl/libxl_migration.c |  3 +-
 2 files changed, 31 insertions(+), 58 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b5101626e..9aa4a293c 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
     }
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
     }
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom)
         goto cleanup;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return type;
 }
 
@@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom)
     ret = virDomainDefGetMemoryTotal(vm->def);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom,
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
     ret = vm->hasManagedSave;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags)
 
  cleanup:
     VIR_FREE(name);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
         ret = virDomainDefGetVcpus(def);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps,
                                            libxl_get_max_cpus(cfg->ctx), NULL);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo,
     ret = maxinfo;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
                              virDomainDefFormatConvertXMLFlags(flags));
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom,
 
  cleanup:
     VIR_FREE(name);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     if (event)
         libxlDomainEventQueue(driver, event);
     virObjectUnref(cfg);
@@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
  cleanup:
     virDomainDefFree(vmdef);
     virDomainDeviceDefFree(dev);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart)
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
     ignore_value(VIR_STRDUP(ret, name));
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -4750,8 +4733,7 @@ libxlDomainOpenConsole(virDomainPtr dom,
     }
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -4886,8 +4868,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom,
     VIR_FREE(nodeset);
     virBitmapFree(nodes);
     libxl_bitmap_dispose(&nodemap);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
@@ -4908,8 +4889,7 @@ libxlDomainIsActive(virDomainPtr dom)
     ret = virDomainObjIsActive(obj);
 
  cleanup:
-    if (obj)
-        virObjectUnlock(obj);
+    virDomainObjEndAPI(&obj);
     return ret;
 }
 
@@ -4928,8 +4908,7 @@ libxlDomainIsPersistent(virDomainPtr dom)
     ret = obj->persistent;
 
  cleanup:
-    if (obj)
-        virObjectUnlock(obj);
+    virDomainObjEndAPI(&obj);
     return ret;
 }
 
@@ -4948,8 +4927,7 @@ libxlDomainIsUpdated(virDomainPtr dom)
     ret = vm->updated;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -5107,8 +5085,7 @@ libxlDomainGetCPUStats(virDomainPtr dom,
                                           start_cpu, ncpus);
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -5211,8 +5188,7 @@ libxlDomainGetJobInfo(virDomainPtr dom,
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -5263,8 +5239,7 @@ libxlDomainGetJobStats(virDomainPtr dom,
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -5897,19 +5872,19 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
     if (STREQ_NULLABLE(vm->def->name, "Domain-0")) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                        _("Domain-0 cannot be migrated"));
-        virObjectUnlock(vm);
+        virDomainObjEndAPI(&vm);
         return NULL;
     }
 
     if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) {
-        virObjectUnlock(vm);
+        virDomainObjEndAPI(&vm);
         return NULL;
     }
 
     if (!virDomainObjIsActive(vm)) {
         virReportError(VIR_ERR_OPERATION_INVALID,
                        "%s", _("domain is not running"));
-        virObjectUnlock(vm);
+        virDomainObjEndAPI(&vm);
         return NULL;
     }
 
@@ -6085,8 +6060,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
     ret = 0;
 
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     return ret;
 }
 
@@ -6174,7 +6148,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
         return -1;
 
     if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) {
-        virObjectUnlock(vm);
+        virDomainObjEndAPI(&vm);
         return -1;
     }
 
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index ccf2daed1..21476f7ac 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -1401,8 +1401,7 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver,
  cleanup:
     if (event)
         libxlDomainEventQueue(driver, event);
-    if (vm)
-        virObjectUnlock(vm);
+    virDomainObjEndAPI(&vm);
     virObjectUnref(cfg);
     return ret;
 }
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] libxl: Properly cleanup after libxlDomObjFromDomain
Posted by Jim Fehlig 7 years, 2 months ago 
On 03/09/2018 09:47 AM, John Ferlan wrote:
> Commit id '9ac945078' altered libxlDomObjFromDomain to return
> a locked *and* ref counted object for some specific purposes;
> however, it neglected to alter all the consumers of the helper
> to use virDomainObjEndAPI thus leaving many objects with extra
> ref counts.
> 
> The two consumers for libxlDomainMigrationConfirm would also
> originally use the libxlDomObjFromDomain API (either from
> libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via
> libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>   src/libxl/libxl_driver.c    | 86 ++++++++++++++++-----------------------------
>   src/libxl/libxl_migration.c |  3 +-
>   2 files changed, 31 insertions(+), 58 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index b5101626e..9aa4a293c 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>       }
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>       }
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom)
>           goto cleanup;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return type;
>   }
>   
> @@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom)
>       ret = virDomainDefGetMemoryTotal(vm->def);
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
>       ret = 0;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom,
>       ret = 0;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
>       ret = vm->hasManagedSave;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags)
>   
>    cleanup:
>       VIR_FREE(name);
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
>           ret = virDomainDefGetVcpus(def);
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps,
>                                              libxl_get_max_cpus(cfg->ctx), NULL);
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo,
>       ret = maxinfo;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
>                                virDomainDefFormatConvertXMLFlags(flags));
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom,
>   
>    cleanup:
>       VIR_FREE(name);
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       if (event)
>           libxlDomainEventQueue(driver, event);
>       virObjectUnref(cfg);
> @@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml,
>    cleanup:
>       virDomainDefFree(vmdef);
>       virDomainDeviceDefFree(dev);
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart)
>       ret = 0;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
>       ignore_value(VIR_STRDUP(ret, name));
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
>       ret = 0;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -4750,8 +4733,7 @@ libxlDomainOpenConsole(virDomainPtr dom,
>       }
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -4886,8 +4868,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom,
>       VIR_FREE(nodeset);
>       virBitmapFree(nodes);
>       libxl_bitmap_dispose(&nodemap);
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;
>   }
> @@ -4908,8 +4889,7 @@ libxlDomainIsActive(virDomainPtr dom)
>       ret = virDomainObjIsActive(obj);
>   
>    cleanup:
> -    if (obj)
> -        virObjectUnlock(obj);
> +    virDomainObjEndAPI(&obj);
>       return ret;
>   }
>   
> @@ -4928,8 +4908,7 @@ libxlDomainIsPersistent(virDomainPtr dom)
>       ret = obj->persistent;
>   
>    cleanup:
> -    if (obj)
> -        virObjectUnlock(obj);
> +    virDomainObjEndAPI(&obj);
>       return ret;
>   }
>   
> @@ -4948,8 +4927,7 @@ libxlDomainIsUpdated(virDomainPtr dom)
>       ret = vm->updated;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -5107,8 +5085,7 @@ libxlDomainGetCPUStats(virDomainPtr dom,
>                                             start_cpu, ncpus);
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -5211,8 +5188,7 @@ libxlDomainGetJobInfo(virDomainPtr dom,
>       ret = 0;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -5263,8 +5239,7 @@ libxlDomainGetJobStats(virDomainPtr dom,
>       ret = 0;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }

For these changes

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

However the migration APIs need some work.

>   
> @@ -5897,19 +5872,19 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>       if (STREQ_NULLABLE(vm->def->name, "Domain-0")) {
>           virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>                          _("Domain-0 cannot be migrated"));
> -        virObjectUnlock(vm);
> +        virDomainObjEndAPI(&vm);
>           return NULL;
>       }
>   
>       if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) {
> -        virObjectUnlock(vm);
> +        virDomainObjEndAPI(&vm);
>           return NULL;
>       }
>   
>       if (!virDomainObjIsActive(vm)) {
>           virReportError(VIR_ERR_OPERATION_INVALID,
>                          "%s", _("domain is not running"));
> -        virObjectUnlock(vm);
> +        virDomainObjEndAPI(&vm);
>           return NULL;
>       }

Not shown here, but the subsequent code calls libxlDomainMigrationBegin which 
unrefs and unlocks on exit. libxlDomainMigrationBegin is also called via 
libxlDomainMigratePerform3Params when doing p2p migration, in which case both 
functions unref/unlock :-(. IMO libxlDomainMigrationBegin should leave 
unref/unlock to its callers, where the ref/locking is done. The same is true for 
libxlDomainMigrationConfirm. I can send a patch to fix these problems, along 
with a few others I noticed when reviewing the migration code.

Regards,
Jim

>   
> @@ -6085,8 +6060,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
>       ret = 0;
>   
>    cleanup:
> -    if (vm)
> -        virObjectUnlock(vm);
> +    virDomainObjEndAPI(&vm);
>       return ret;
>   }
>   
> @@ -6174,7 +6148,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
>           return -1;
>   
>       if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) {
> -        virObjectUnlock(vm);
> +        virDomainObjEndAPI(&vm);
>           return -1;
>       }
>   
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index ccf2daed1..21476f7ac 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -1401,8 +1401,7 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver,
>    cleanup:
>       if (event)
>           libxlDomainEventQueue(driver, event);
> -    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 03/20] libxl: Properly cleanup after libxlDomObjFromDomain
Posted by John Ferlan 7 years, 2 months ago 

On 03/11/2018 10:13 PM, Jim Fehlig wrote:
> On 03/09/2018 09:47 AM, John Ferlan wrote:
>> Commit id '9ac945078' altered libxlDomObjFromDomain to return
>> a locked *and* ref counted object for some specific purposes;
>> however, it neglected to alter all the consumers of the helper
>> to use virDomainObjEndAPI thus leaving many objects with extra
>> ref counts.
>>
>> The two consumers for libxlDomainMigrationConfirm would also
>> originally use the libxlDomObjFromDomain API (either from
>> libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via
>> libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>   src/libxl/libxl_driver.c    | 86
>> ++++++++++++++++-----------------------------
>>   src/libxl/libxl_migration.c |  3 +-
>>   2 files changed, 31 insertions(+), 58 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index b5101626e..9aa4a293c 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c

[...]

> 
> For these changes
> 
> Reviewed-by: Jim Fehlig <jfehlig@suse.com>
> 

Tks -

> However the migration APIs need some work.
> 
>>   @@ -5897,19 +5872,19 @@ libxlDomainMigrateBegin3Params(virDomainPtr
>> domain,
>>       if (STREQ_NULLABLE(vm->def->name, "Domain-0")) {
>>           virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>                          _("Domain-0 cannot be migrated"));
>> -        virObjectUnlock(vm);
>> +        virDomainObjEndAPI(&vm);
>>           return NULL;
>>       }
>>         if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn,
>> vm->def) < 0) {
>> -        virObjectUnlock(vm);
>> +        virDomainObjEndAPI(&vm);
>>           return NULL;
>>       }
>>         if (!virDomainObjIsActive(vm)) {
>>           virReportError(VIR_ERR_OPERATION_INVALID,
>>                          "%s", _("domain is not running"));
>> -        virObjectUnlock(vm);
>> +        virDomainObjEndAPI(&vm);
>>           return NULL;
>>       }
> 
> Not shown here, but the subsequent code calls libxlDomainMigrationBegin
> which unrefs and unlocks on exit. libxlDomainMigrationBegin is also
> called via libxlDomainMigratePerform3Params when doing p2p migration, in
> which case both functions unref/unlock :-(. IMO
> libxlDomainMigrationBegin should leave unref/unlock to its callers,
> where the ref/locking is done. The same is true for
> libxlDomainMigrationConfirm. I can send a patch to fix these problems,
> along with a few others I noticed when reviewing the migration code.
> 
> Regards,
> Jim
> 

Hmm... right, I noticed that the migration API's were treating @vm a bit
oddly. I looked backwards through the call stack figuring I'd got things
right, but certainly having libxlDomainMigrationBegin do that EndAPI is
not good!

I figure getting all these patches reviewed could take some time, so if
you want to clean up the libxlDomainMigration paths as you note that'd
be fine. If it becomes problematic trying to work thru things without
pushing what's R-B'd so far in libxl, I can do so. I can also merge in
what you can get done into my changes.  Whatever works best.

Tks -

John



>>   @@ -6085,8 +6060,7 @@ libxlDomainMigratePerform3Params(virDomainPtr
>> dom,
>>       ret = 0;
>>      cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>>   @@ -6174,7 +6148,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr
>> domain,
>>           return -1;
>>         if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn,
>> vm->def) < 0) {
>> -        virObjectUnlock(vm);
>> +        virDomainObjEndAPI(&vm);
>>           return -1;
>>       }
>>   diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index ccf2daed1..21476f7ac 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -1401,8 +1401,7 @@
>> libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver,
>>    cleanup:
>>       if (event)
>>           libxlDomainEventQueue(driver, event);
>> -    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 03/20] libxl: Properly cleanup after libxlDomObjFromDomain
Posted by Jim Fehlig 7 years, 1 month ago 
On 03/11/2018 08:13 PM, Jim Fehlig wrote:
> On 03/09/2018 09:47 AM, John Ferlan wrote:
>> Commit id '9ac945078' altered libxlDomObjFromDomain to return
>> a locked *and* ref counted object for some specific purposes;
>> however, it neglected to alter all the consumers of the helper
>> to use virDomainObjEndAPI thus leaving many objects with extra
>> ref counts.
>>
>> The two consumers for libxlDomainMigrationConfirm would also
>> originally use the libxlDomObjFromDomain API (either from
>> libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via
>> libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> ---
>>   src/libxl/libxl_driver.c    | 86 ++++++++++++++++-----------------------------
>>   src/libxl/libxl_migration.c |  3 +-
>>   2 files changed, 31 insertions(+), 58 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index b5101626e..9aa4a293c 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int 
>> flags)
>>       }
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>>       }
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom)
>>           goto cleanup;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return type;
>>   }
>> @@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom)
>>       ret = virDomainDefGetMemoryTotal(vm->def);
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
>>       ret = 0;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom,
>>       ret = 0;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, 
>> unsigned int flags)
>>       ret = vm->hasManagedSave;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned 
>> int flags)
>>    cleanup:
>>       VIR_FREE(name);
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int 
>> flags)
>>           ret = virDomainDefGetVcpus(def);
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps,
>>                                              libxl_get_max_cpus(cfg->ctx), NULL);
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr 
>> info, int maxinfo,
>>       ret = maxinfo;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
>>                                virDomainDefFormatConvertXMLFlags(flags));
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom,
>>    cleanup:
>>       VIR_FREE(name);
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       if (event)
>>           libxlDomainEventQueue(driver, event);
>>       virObjectUnref(cfg);
>> @@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const 
>> char *xml,
>>    cleanup:
>>       virDomainDefFree(vmdef);
>>       virDomainDeviceDefFree(dev);
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart)
>>       ret = 0;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
>>       ignore_value(VIR_STRDUP(ret, name));
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom,
>>       ret = 0;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -4750,8 +4733,7 @@ libxlDomainOpenConsole(virDomainPtr dom,
>>       }
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -4886,8 +4868,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom,
>>       VIR_FREE(nodeset);
>>       virBitmapFree(nodes);
>>       libxl_bitmap_dispose(&nodemap);
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       virObjectUnref(cfg);
>>       return ret;
>>   }
>> @@ -4908,8 +4889,7 @@ libxlDomainIsActive(virDomainPtr dom)
>>       ret = virDomainObjIsActive(obj);
>>    cleanup:
>> -    if (obj)
>> -        virObjectUnlock(obj);
>> +    virDomainObjEndAPI(&obj);
>>       return ret;
>>   }
>> @@ -4928,8 +4908,7 @@ libxlDomainIsPersistent(virDomainPtr dom)
>>       ret = obj->persistent;
>>    cleanup:
>> -    if (obj)
>> -        virObjectUnlock(obj);
>> +    virDomainObjEndAPI(&obj);
>>       return ret;
>>   }
>> @@ -4948,8 +4927,7 @@ libxlDomainIsUpdated(virDomainPtr dom)
>>       ret = vm->updated;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -5107,8 +5085,7 @@ libxlDomainGetCPUStats(virDomainPtr dom,
>>                                             start_cpu, ncpus);
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -5211,8 +5188,7 @@ libxlDomainGetJobInfo(virDomainPtr dom,
>>       ret = 0;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
>> @@ -5263,8 +5239,7 @@ libxlDomainGetJobStats(virDomainPtr dom,
>>       ret = 0;
>>    cleanup:
>> -    if (vm)
>> -        virObjectUnlock(vm);
>> +    virDomainObjEndAPI(&vm);
>>       return ret;
>>   }
> 
> For these changes
> 
> Reviewed-by: Jim Fehlig <jfehlig@suse.com>

I've pushed this part of the patch. Your fixes have encouraged me to audit all 
the begin/end API code in the libxl driver and I'd like to base any fixes on top 
of items we've already hashed out.

Regards,
Jim

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.