4:58 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1718707
When restoring a domain we might leave a qemu process behind.
The problem is described in 4/4 in more detail, but the digest is: we
start qemu process successfully but then fail to restore its vCPUs and
leave qemu process running and forget about. I thought of two possible
ways to fix this:
1) Kill qemu process and claim error.
2) Don't kill qemu process and don't remove it from our internal list
and claim error.
I lean towards 1) because I find it clearer. If I were a mgmt app and
call 'virsh restore /path/to/domain.save' then I'd much rather see
either success or an error without any need for cleanup (which would be
implied if we went with number 2).
Michal Prívozník (4):
qemuDomainSaveImageStartVM: Use VIR_AUTOCLOSE for @intermediatefd
qemuDomainSaveImageStartVM: Use g_autoptr() for virCommand
qemu: Use g_autoptr() for qemuDomainSaveCookie
qemu: Stop domain on failed restore
src/qemu/qemu_domain.c | 28 ++++++++++------------------
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 37 +++++++++++++++++--------------------
3 files changed, 28 insertions(+), 38 deletions(-)
--
2.24.1
4:58 a.m.
New subject: [libvirt] [PATCH 1/4] qemuDomainSaveImageStartVM: Use VIR_AUTOCLOSE for @intermediatefd
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d6b1e9f00c..9de5bc6a32 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6803,7 +6803,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
int ret = -1;
bool restored = false;
virObjectEventPtr event;
- int intermediatefd = -1;
+ VIR_AUTOCLOSE intermediatefd = -1;
virCommandPtr cmd = NULL;
g_autofree char *errbuf = NULL;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -6829,6 +6829,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
if (virCommandRunAsync(cmd, NULL) < 0) {
*fd = intermediatefd;
+ intermediatefd = -1;
goto cleanup;
}
}
@@ -6872,8 +6873,6 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
virErrorRestore(&orig_err);
}
- VIR_FORCE_CLOSE(intermediatefd);
-
if (VIR_CLOSE(*fd) < 0) {
virReportSystemError(errno, _("cannot close file: %s"), path);
restored = false;
--
2.24.1
4:58 a.m.
New subject: [libvirt] [PATCH 2/4] qemuDomainSaveImageStartVM: Use g_autoptr() for virCommand
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9de5bc6a32..e1c0550b9a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6804,7 +6804,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
bool restored = false;
virObjectEventPtr event;
VIR_AUTOCLOSE intermediatefd = -1;
- virCommandPtr cmd = NULL;
+ g_autoptr(virCommand) cmd = NULL;
g_autofree char *errbuf = NULL;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
virQEMUSaveHeaderPtr header = &data->header;
@@ -6920,7 +6920,6 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
cleanup:
virObjectUnref(cookie);
- virCommandFree(cmd);
if (qemuSecurityRestoreSavedStateLabel(driver, vm, path) < 0)
VIR_WARN("failed to restore save state label on %s", path);
return ret;
--
2.24.1
4:58 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Use g_autoptr() for qemuDomainSaveCookie
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 28 ++++++++++------------------
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 6 ++----
3 files changed, 13 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1f358544ab..3620739120 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15997,27 +15997,23 @@ qemuDomainSaveCookiePtr
qemuDomainSaveCookieNew(virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- qemuDomainSaveCookiePtr cookie = NULL;
+ g_autoptr(qemuDomainSaveCookie) cookie = NULL;
if (qemuDomainInitialize() < 0)
- goto error;
+ return NULL;
if (!(cookie = virObjectNew(qemuDomainSaveCookieClass)))
- goto error;
+ return NULL;
if (priv->origCPU && !(cookie->cpu =
virCPUDefCopy(vm->def->cpu)))
- goto error;
+ return NULL;
cookie->slirpHelper = qemuDomainGetSlirpHelperOk(vm);
VIR_DEBUG("Save cookie %p, cpu=%p, slirpHelper=%d",
cookie, cookie->cpu, cookie->slirpHelper);
- return cookie;
-
- error:
- virObjectUnref(cookie);
- return NULL;
+ return g_steal_pointer(&cookie);
}
@@ -16025,26 +16021,22 @@ static int
qemuDomainSaveCookieParse(xmlXPathContextPtr ctxt G_GNUC_UNUSED,
virObjectPtr *obj)
{
- qemuDomainSaveCookiePtr cookie = NULL;
+ g_autoptr(qemuDomainSaveCookie) cookie = NULL;
if (qemuDomainInitialize() < 0)
- goto error;
+ return -1;
if (!(cookie = virObjectNew(qemuDomainSaveCookieClass)))
- goto error;
+ return -1;
if (virCPUDefParseXML(ctxt, "./cpu[1]", VIR_CPU_TYPE_GUEST,
&cookie->cpu) < 0)
- goto error;
+ return -1;
cookie->slirpHelper = virXPathBoolean("boolean(./slirpHelper)", ctxt)
> 0;
- *obj = (virObjectPtr) cookie;
+ *obj = (virObjectPtr) g_steal_pointer(&cookie);
return 0;
-
- error:
- virObjectUnref(cookie);
- return -1;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index c6afc484f6..60b80297fa 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -610,6 +610,7 @@ struct _qemuDomainSaveCookie {
bool slirpHelper;
};
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDomainSaveCookie, virObjectUnref);
typedef struct _qemuDomainXmlNsDef qemuDomainXmlNsDef;
typedef qemuDomainXmlNsDef *qemuDomainXmlNsDefPtr;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e1c0550b9a..ce9b1772c1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3293,7 +3293,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
virObjectEventPtr event = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
virQEMUSaveDataPtr data = NULL;
- qemuDomainSaveCookiePtr cookie = NULL;
+ g_autoptr(qemuDomainSaveCookie) cookie = NULL;
if (!qemuMigrationSrcIsAllowed(driver, vm, false, 0))
goto cleanup;
@@ -3399,7 +3399,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
qemuDomainRemoveInactiveJob(driver, vm);
cleanup:
- virObjectUnref(cookie);
virQEMUSaveDataFree(data);
virObjectEventStateQueue(driver->domainEventState, event);
return ret;
@@ -6808,7 +6807,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
g_autofree char *errbuf = NULL;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
virQEMUSaveHeaderPtr header = &data->header;
- qemuDomainSaveCookiePtr cookie = NULL;
+ g_autoptr(qemuDomainSaveCookie) cookie = NULL;
if (virSaveCookieParseString(data->cookie, (virObjectPtr *)&cookie,
virDomainXMLOptionGetSaveCookie(driver->xmlopt)) <
0)
@@ -6919,7 +6918,6 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
ret = 0;
cleanup:
- virObjectUnref(cookie);
if (qemuSecurityRestoreSavedStateLabel(driver, vm, path) < 0)
VIR_WARN("failed to restore save state label on %s", path);
return ret;
--
2.24.1
4:58 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: Stop domain on failed restore
When resuming a domain from a save file, we read the domain XML
from the file, add it onto our internal list of domains, start
the qemu process, let it load the incoming migration stream and
resume its vCPUs afterwards. If anything goes wrong, the domain
object is removed from the list of domains and error is returned
to the caller. However, the qemu process might be left behind -
if resuming vCPUs fails (e.g. because qemu is unable to acquire
write lock on a disk) then due to a bug the qemu process is not
killed but the domain object is removed from the list.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1718707
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ce9b1772c1..217d873671 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6800,7 +6800,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
int ret = -1;
- bool restored = false;
+ bool started = false;
virObjectEventPtr event;
VIR_AUTOCLOSE intermediatefd = -1;
g_autoptr(virCommand) cmd = NULL;
@@ -6808,6 +6808,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
virQEMUSaveHeaderPtr header = &data->header;
g_autoptr(qemuDomainSaveCookie) cookie = NULL;
+ int rc = 0;
if (virSaveCookieParseString(data->cookie, (virObjectPtr *)&cookie,
virDomainXMLOptionGetSaveCookie(driver->xmlopt)) <
0)
@@ -6848,12 +6849,12 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
VIR_QEMU_PROCESS_START_PAUSED |
VIR_QEMU_PROCESS_START_GEN_VMID) == 0)
- restored = true;
+ started = true;
if (intermediatefd != -1) {
virErrorPtr orig_err = NULL;
- if (!restored) {
+ if (!started) {
/* if there was an error setting up qemu, the intermediate
* process will wait forever to write to stdout, so we
* must manually kill it and ignore any error related to
@@ -6864,21 +6865,17 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
VIR_FORCE_CLOSE(*fd);
}
- if (virCommandWait(cmd, NULL) < 0) {
- qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, asyncJob, 0);
- restored = false;
- }
+ rc = virCommandWait(cmd, NULL);
VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
-
virErrorRestore(&orig_err);
}
if (VIR_CLOSE(*fd) < 0) {
virReportSystemError(errno, _("cannot close file: %s"), path);
- restored = false;
+ rc = -1;
}
- virDomainAuditStart(vm, "restored", restored);
- if (!restored)
+ virDomainAuditStart(vm, "restored", started);
+ if (!started || rc < 0)
goto cleanup;
/* qemuProcessStart doesn't unset the qemu error reporting infrastructure
@@ -6918,6 +6915,10 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
ret = 0;
cleanup:
+ if (ret < 0 && started) {
+ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
+ asyncJob, VIR_QEMU_PROCESS_STOP_MIGRATED);
+ }
if (qemuSecurityRestoreSavedStateLabel(driver, vm, path) < 0)
VIR_WARN("failed to restore save state label on %s", path);
return ret;
--
2.24.1
9:14 a.m.
On 1/13/20 7:58 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1718707
When restoring a domain we might leave a qemu process behind.
The problem is described in 4/4 in more detail, but the digest is: we
start qemu process successfully but then fail to restore its vCPUs and
leave qemu process running and forget about. I thought of two possible
ways to fix this:
1) Kill qemu process and claim error.
2) Don't kill qemu process and don't remove it from our internal list
and claim error.
I lean towards 1) because I find it clearer. If I were a mgmt app and
call 'virsh restore /path/to/domain.save' then I'd much rather see
either success or an error without any need for cleanup (which would be
implied if we went with number 2).
Michal Prívozník (4):
qemuDomainSaveImageStartVM: Use VIR_AUTOCLOSE for @intermediatefd
qemuDomainSaveImageStartVM: Use g_autoptr() for virCommand
qemu: Use g_autoptr() for qemuDomainSaveCookie
qemu: Stop domain on failed restore
All patches:
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
src/qemu/qemu_domain.c | 28 ++++++++++------------------
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 37 +++++++++++++++++--------------------
3 files changed, 28 insertions(+), 38 deletions(-)
1775
days inactive
1775
days old
5 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik