[libvirt] [PATCH 0/3] Fix libvirtd crash when starting two managedsave operations on a single domain
8:44 a.m.
We had an unfortunate code path where when you'd start two managedsave operations
simultaneously libvirtd would crash. Fix it by adding a VM liveness check.
Peter Krempa (3):
qemu: managedsave: Check that VM is alive after entering async job
security: selinux: Set saved state label only if it is available
qemu: migration: Check domain live state after exitting the monitor
src/qemu/qemu_driver.c | 6 ++++++
src/qemu/qemu_migration.c | 19 +++++++++++++++++--
src/security/security_selinux.c | 2 +-
3 files changed, 24 insertions(+), 3 deletions(-)
--
2.0.2
8:44 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: managedsave: Check that VM is alive after entering async job
Saving a shutoff VM doesn't make sense (and has suboptimal effects on
stability of the libvirt daemon). Check that the domain is alive after
entering the save async job.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1129207
---
src/qemu/qemu_driver.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2c3f179..b6219ba 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3096,6 +3096,12 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
if (qemuDomainObjBeginAsyncJob(driver, vm, QEMU_ASYNC_JOB_SAVE) < 0)
goto cleanup;
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto endjob;
+ }
+
memset(&priv->job.info, 0, sizeof(priv->job.info));
priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED;
--
2.0.2
10:17 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: managedsave: Check that VM is alive after entering async job
On 08/12/2014 03:44 PM, Peter Krempa wrote:
Saving a shutoff VM doesn't make sense (and has suboptimal
effects on
stability of the libvirt daemon). Check that the domain is alive after
Saying it 'crashes' is more readable IMHO.
entering the save async job.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1129207
---
src/qemu/qemu_driver.c | 6 ++++++
1 file changed, 6 insertions(+)
ACK
8:44 a.m.
New subject: [libvirt] [PATCH 2/3] security: selinux: Set saved state label only if it is available
Check that secdef->imagelabel exists before blindly applying it.
---
src/security/security_selinux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c078cab..cf59d6c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1962,7 +1962,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
virSecurityLabelDefPtr secdef;
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
- if (!secdef || !secdef->relabel)
+ if (!secdef || !secdef->imagelabel || !secdef->relabel)
return 0;
return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel);
--
2.0.2
10:17 a.m.
New subject: [libvirt] [PATCH 2/3] security: selinux: Set saved state label only if it is available
On 08/12/2014 03:44 PM, Peter Krempa wrote:
Check that secdef->imagelabel exists before blindly applying it.
---
src/security/security_selinux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index c078cab..cf59d6c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1962,7 +1962,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
virSecurityLabelDefPtr secdef;
secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
- if (!secdef || !secdef->relabel)
+ if (!secdef || !secdef->imagelabel || !secdef->relabel)
return 0;
return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel);
I think this is just covers up incorrect usage of this function on shutoff
domains, because if a running domain has a selinux label, it should have the
imagelabel generated.
Jan
10:30 a.m.
New subject: [libvirt] [PATCH 2/3] security: selinux: Set saved state label only if it is available
On 08/12/14 17:17, Ján Tomko wrote:
On 08/12/2014 03:44 PM, Peter Krempa wrote:
> Check that secdef->imagelabel exists before blindly applying it.
> ---
> src/security/security_selinux.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index c078cab..cf59d6c 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1962,7 +1962,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
> virSecurityLabelDefPtr secdef;
>
> secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
> - if (!secdef || !secdef->relabel)
> + if (!secdef || !secdef->imagelabel || !secdef->relabel)
> return 0;
>
> return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel);
>
I think this is just covers up incorrect usage of this function on shutoff
domains, because if a running domain has a selinux label, it should have the
imagelabel generated.
Fair enough. I'll drop this one.
8:44 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: migration: Check domain live state after exitting the monitor
In qemuMigrationToFile we enter the monitor multiple times and don't
check if the VM is still alive after returning form the monitor. Add the
checks to skip pieces of code in case the VM crashes while saving it's
state.
---
src/qemu/qemu_migration.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 767d840..50a1eab 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4731,6 +4731,13 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
qemuDomainObjExitMonitor(driver, vm);
}
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ /* nothing to tear down */
+ return -1;
+ }
+
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
(!compressor || pipe(pipeFD) == 0)) {
/* All right! We can use fd migration, which means that qemu
@@ -4818,6 +4825,12 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
}
qemuDomainObjExitMonitor(driver, vm);
+ if (!virDomainObjIsActive(vm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("guest unexpectedly quit"));
+ goto cleanup;
+ }
+
if (rc < 0)
goto cleanup;
@@ -4827,7 +4840,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
if (rc == -2) {
orig_err = virSaveLastError();
virCommandAbort(cmd);
- if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
+ if (virDomainObjIsActive(vm) &&
+ qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
qemuMonitorMigrateCancel(priv->mon);
qemuDomainObjExitMonitor(driver, vm);
}
@@ -4845,7 +4859,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
orig_err = virSaveLastError();
/* Restore max migration bandwidth */
- if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
+ if (virDomainObjIsActive(vm) &&
+ qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
qemuMonitorSetMigrationSpeed(priv->mon, saveMigBandwidth);
priv->migMaxBandwidth = saveMigBandwidth;
qemuDomainObjExitMonitor(driver, vm);
--
2.0.2
10:17 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: migration: Check domain live state after exitting the monitor
On 08/12/2014 03:44 PM, Peter Krempa wrote:
In qemuMigrationToFile we enter the monitor multiple times and
don't
check if the VM is still alive after returning form the monitor. Add the
checks to skip pieces of code in case the VM crashes while saving it's
state.
---
src/qemu/qemu_migration.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
ACK
Jan
10:35 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: migration: Check domain live state after exitting the monitor
On 08/12/14 17:17, Ján Tomko wrote:
On 08/12/2014 03:44 PM, Peter Krempa wrote:
> In qemuMigrationToFile we enter the monitor multiple times and don't
> check if the VM is still alive after returning form the monitor. Add the
> checks to skip pieces of code in case the VM crashes while saving it's
> state.
> ---
> src/qemu/qemu_migration.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
ACK
I've tweaked the commit message in 1/3 and pushed 1/3 and 3/3.
Thanks.
Peter
11:33 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: migration: Check domain live state after exitting the monitor
On 08/12/2014 07:44 AM, Peter Krempa wrote:
s/exitting/exiting/ in the title
In qemuMigrationToFile we enter the monitor multiple times and
don't
check if the VM is still alive after returning form the monitor. Add the
checks to skip pieces of code in case the VM crashes while saving it's
s/it's/its/ ("it's" is only correct if you can replace it with "it
is")
state.
Oh well, I see this was already pushed; so we'll live with the typos :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3755
days inactive
3755
days old
9 comments
3 participants
participants (3)
-
Eric Blake -
Ján Tomko -
Peter Krempa