[libvirt] [PATCH 03/20] libxl: Properly cleanup after libxlDomObjFromDomain
Jim Fehlig
jfehlig at suse.com
Mon Mar 12 02:13:05 UTC 2018
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 at 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 at 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;
> }
>
More information about the libvir-list
mailing list