2:54 a.m.
libxlDomainStart allocates and reserves resources, which should be cleaned
up on error paths. This small series is based on Chunyan's patch to do the
same [0]. It breaks the patch into three patches to ease review and better
understand the changes.
[0] https://www.redhat.com/archives/libvir-list/2016-March/msg00929.html
Chunyan Liu (1):
libxl: fix resource leaks in libxlDomainStart error paths
Jim Fehlig (2):
libxl: rename cleanup_dom label
libxl: only disable domain death events in libxlDomainCleanup
src/libxl/libxl_domain.c | 44 ++++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 24 deletions(-)
--
2.1.4
2:54 a.m.
New subject: [libvirt] [PATCH 1/3] libxl: rename cleanup_dom label
Rename cleanup_dom label to destroy_dom, which better describes what
it does.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_domain.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index c8d09b1..ead3d09 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1102,22 +1102,22 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr
vm,
/* Always enable domain death events */
if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0,
&priv->deathW))
- goto cleanup_dom;
+ goto destroy_dom;
libxlDomainCreateIfaceNames(vm->def, &d_config);
if ((dom_xml = virDomainDefFormat(vm->def, cfg->caps, 0)) == NULL)
- goto cleanup_dom;
+ goto destroy_dom;
if (libxl_userdata_store(cfg->ctx, domid, "libvirt-xml",
(uint8_t *)dom_xml, strlen(dom_xml) + 1)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("libxenlight failed to store userdata"));
- goto cleanup_dom;
+ goto destroy_dom;
}
if (libxlDomainSetVcpuAffinities(driver, vm) < 0)
- goto cleanup_dom;
+ goto destroy_dom;
if (!start_paused) {
libxl_domain_unpause(cfg->ctx, domid);
@@ -1127,7 +1127,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
}
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, cfg->caps) <
0)
- goto cleanup_dom;
+ goto destroy_dom;
if (virAtomicIntInc(&driver->nactive) == 1 &&
driver->inhibitCallback)
driver->inhibitCallback(true, driver->inhibitOpaque);
@@ -1142,7 +1142,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
ret = 0;
goto cleanup;
- cleanup_dom:
+ destroy_dom:
ret = -1;
if (priv->deathW) {
libxl_evdisable_domain_death(cfg->ctx, priv->deathW);
--
2.1.4
3:35 a.m.
New subject: [libvirt] [PATCH 1/3] libxl: rename cleanup_dom label
Reviewed-by: Chunyan Liu <cyliu(a)suse.com>
>> On 3/29/2016 at 08:54 AM, in message
<1459212889-5490-2-git-send-email-jfehlig(a)suse.com>, Jim Fehlig
<jfehlig(a)suse.com> wrote:
Rename cleanup_dom label to destroy_dom, which better describes what
it does.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_domain.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index c8d09b1..ead3d09 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1102,22 +1102,22 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
/* Always enable domain death events */
if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0,
&priv->deathW))
- goto cleanup_dom;
+ goto destroy_dom;
libxlDomainCreateIfaceNames(vm->def, &d_config);
if ((dom_xml = virDomainDefFormat(vm->def, cfg->caps, 0)) == NULL)
- goto cleanup_dom;
+ goto destroy_dom;
if (libxl_userdata_store(cfg->ctx, domid, "libvirt-xml",
(uint8_t *)dom_xml, strlen(dom_xml) + 1)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("libxenlight failed to store userdata"));
- goto cleanup_dom;
+ goto destroy_dom;
}
if (libxlDomainSetVcpuAffinities(driver, vm) < 0)
- goto cleanup_dom;
+ goto destroy_dom;
if (!start_paused) {
libxl_domain_unpause(cfg->ctx, domid);
@@ -1127,7 +1127,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
}
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, cfg->caps) <
0)
- goto cleanup_dom;
+ goto destroy_dom;
if (virAtomicIntInc(&driver->nactive) == 1 &&
driver->inhibitCallback)
driver->inhibitCallback(true, driver->inhibitOpaque);
@@ -1142,7 +1142,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
ret = 0;
goto cleanup;
- cleanup_dom:
+ destroy_dom:
ret = -1;
if (priv->deathW) {
libxl_evdisable_domain_death(cfg->ctx, priv->deathW);
2:54 a.m.
New subject: [libvirt] [PATCH 2/3] libxl: fix resource leaks in libxlDomainStart error paths
From: Chunyan Liu <cyliu(a)suse.com>
libxlDomainStart allocates and reserves resources that were not
being released in error paths. libxlDomainCleanup already handles
the job of releasing resources, and libxlDomainStart should call
it when encountering a failure.
Change the error handling logic to call libxlDomainCleanup on
failure. This includes acquiring the lease sooner and allowing
it to be released in libxlDomainCleanup on failure, similar to
the way other resources are reclaimed. With the lease now
released in libxlDomainCleanup, the release_dom label can be
renamed to cleanup_dom to better reflect its changed semantics.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_domain.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ead3d09..068bfb6 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1036,17 +1036,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
vm, true) < 0)
goto cleanup;
- if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
- cfg->ctx, &d_config) < 0)
- goto cleanup;
-
- if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config)
< 0)
- goto cleanup;
-
- if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
- vm->def, VIR_HOSTDEV_SP_PCI) < 0)
- goto cleanup;
-
if (virDomainLockProcessStart(driver->lockManager,
"xen:///system",
vm,
@@ -1061,6 +1050,17 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
goto cleanup;
VIR_FREE(priv->lockState);
+ if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
+ cfg->ctx, &d_config) < 0)
+ goto cleanup_dom;
+
+ if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config)
< 0)
+ goto cleanup_dom;
+
+ if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
+ vm->def, VIR_HOSTDEV_SP_PCI) < 0)
+ goto cleanup_dom;
+
/* Unlock virDomainObj while creating the domain */
virObjectUnlock(vm);
@@ -1091,7 +1091,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight failed to restore domain
'%s'"),
d_config.c_info.name);
- goto release_dom;
+ goto cleanup_dom;
}
/*
@@ -1152,8 +1152,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
vm->def->id = -1;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED);
- release_dom:
- virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState);
+ cleanup_dom:
+ libxlDomainCleanup(driver, vm);
cleanup:
libxl_domain_config_dispose(&d_config);
--
2.1.4
3:36 a.m.
New subject: [libvirt] [PATCH 2/3] libxl: fix resource leaks in libxlDomainStart error paths
Reviewed-by: Chunyan Liu <cyliu(a)suse.com>
>> On 3/29/2016 at 08:54 AM, in message
<1459212889-5490-3-git-send-email-jfehlig(a)suse.com>, Jim Fehlig
<jfehlig(a)suse.com> wrote:
From: Chunyan Liu <cyliu(a)suse.com>
libxlDomainStart allocates and reserves resources that were not
being released in error paths. libxlDomainCleanup already handles
the job of releasing resources, and libxlDomainStart should call
it when encountering a failure.
Change the error handling logic to call libxlDomainCleanup on
failure. This includes acquiring the lease sooner and allowing
it to be released in libxlDomainCleanup on failure, similar to
the way other resources are reclaimed. With the lease now
released in libxlDomainCleanup, the release_dom label can be
renamed to cleanup_dom to better reflect its changed semantics.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_domain.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ead3d09..068bfb6 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1036,17 +1036,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
vm, true) < 0)
goto cleanup;
- if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
- cfg->ctx, &d_config) < 0)
- goto cleanup;
-
- if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config)
< 0)
- goto cleanup;
-
- if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
- vm->def, VIR_HOSTDEV_SP_PCI) < 0)
- goto cleanup;
-
if (virDomainLockProcessStart(driver->lockManager,
"xen:///system",
vm,
@@ -1061,6 +1050,17 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
goto cleanup;
VIR_FREE(priv->lockState);
+ if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
+ cfg->ctx, &d_config) < 0)
+ goto cleanup_dom;
+
+ if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config)
< 0)
+ goto cleanup_dom;
+
+ if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
+ vm->def, VIR_HOSTDEV_SP_PCI) < 0)
+ goto cleanup_dom;
+
/* Unlock virDomainObj while creating the domain */
virObjectUnlock(vm);
@@ -1091,7 +1091,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight failed to restore domain
'%s'"),
d_config.c_info.name);
- goto release_dom;
+ goto cleanup_dom;
}
/*
@@ -1152,8 +1152,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
vm->def->id = -1;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,
VIR_DOMAIN_SHUTOFF_FAILED);
- release_dom:
- virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState);
+ cleanup_dom:
+ libxlDomainCleanup(driver, vm);
cleanup:
libxl_domain_config_dispose(&d_config);
2:54 a.m.
New subject: [libvirt] [PATCH 3/3] libxl: only disable domain death events in libxlDomainCleanup
Remove disabling domain death events from libxlDomainStart error
path. The domain death event is already disabled in libxlDomainCleanup.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_domain.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 068bfb6..04962a0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1144,10 +1144,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
destroy_dom:
ret = -1;
- if (priv->deathW) {
- libxl_evdisable_domain_death(cfg->ctx, priv->deathW);
- priv->deathW = NULL;
- }
libxlDomainDestroyInternal(driver, vm);
vm->def->id = -1;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED);
--
2.1.4
3:36 a.m.
New subject: [libvirt] [PATCH 3/3] libxl: only disable domain death events in libxlDomainCleanup
Reviewed-by: Chunyan Liu <cyliu(a)suse.com>
>> On 3/29/2016 at 08:54 AM, in message
<1459212889-5490-4-git-send-email-jfehlig(a)suse.com>, Jim Fehlig
<jfehlig(a)suse.com> wrote:
Remove disabling domain death events from libxlDomainStart error
path. The domain death event is already disabled in libxlDomainCleanup.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_domain.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 068bfb6..04962a0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1144,10 +1144,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
destroy_dom:
ret = -1;
- if (priv->deathW) {
- libxl_evdisable_domain_death(cfg->ctx, priv->deathW);
- priv->deathW = NULL;
- }
libxlDomainDestroyInternal(driver, vm);
vm->def->id = -1;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF,
VIR_DOMAIN_SHUTOFF_FAILED);
3096
days inactive
3096
days old
6 comments
2 participants
participants (2)
-
Chun Yan Liu -
Jim Fehlig