[libvirt] [PATCH 0/3] qemu: monitor: detect more snapshot errors
qemu in Fedora 30 rejects migration if the VM has nested VMX configured. This means managedsave and snapshots are rejected too. Unfortunately the scraping we need to resort to with the text monitor snapshot commands is not detecting that 'savevm' actually failed, so the snapshot appears to succeed. https://bugzilla.redhat.com/show_bug.cgi?id=1697997 qemu 4.0.0 will add an 'Error: ' prefix to most hmp command errors (qemu commit 66363e9a43f), let's use that to detect error as well Cole Robinson (3): qemu: monitor: cleanup loadvm error handling qemu: monitor cleanup delvm error handling qemu: monitor: check for common 'Error: ' string src/qemu/qemu_monitor_text.c | 39 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) -- 2.21.0
Drop redundant NULL checks, add error string prefixes, consolidate a few indentical reports. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_monitor_text.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 0b2c1a6aed..414da0ca28 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -215,28 +215,25 @@ int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const char *name) if (qemuMonitorHMPCommand(mon, cmd, &reply)) goto cleanup; - if (strstr(reply, "No block device supports snapshots") != NULL) { + if (strstr(reply, "No block device supports snapshots")) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("this domain does not have a device to load snapshots")); goto cleanup; - } else if (strstr(reply, "Could not find snapshot") != NULL) { + } else if (strstr(reply, "Could not find snapshot")) { virReportError(VIR_ERR_OPERATION_INVALID, _("the snapshot '%s' does not exist, and was not loaded"), name); goto cleanup; - } else if (strstr(reply, "Snapshots not supported on device") != NULL) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", reply); - goto cleanup; - } else if (strstr(reply, "Could not open VM state file") != NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); - goto cleanup; - } else if (strstr(reply, "Error") != NULL - && strstr(reply, "while loading VM state") != NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); + } else if (strstr(reply, "Snapshots not supported on device")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Failed to load snapshot: %s"), reply); goto cleanup; - } else if (strstr(reply, "Error") != NULL - && strstr(reply, "while activating snapshot on") != NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); + } else if (strstr(reply, "Could not open VM state file") || + (strstr(reply, "Error") && + (strstr(reply, "while loading VM state") || + strstr(reply, "while activating snapshot on")))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to load snapshot: %s"), reply); goto cleanup; } -- 2.21.0
Drop redundant NULL checks, and add an error string prefix Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_monitor_text.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 414da0ca28..76095674ab 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -259,16 +259,17 @@ int qemuMonitorTextDeleteSnapshot(qemuMonitorPtr mon, const char *name) if (qemuMonitorHMPCommand(mon, cmd, &reply)) goto cleanup; - if (strstr(reply, "No block device supports snapshots") != NULL) { + if (strstr(reply, "No block device supports snapshots")) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("this domain does not have a device to delete snapshots")); goto cleanup; - } else if (strstr(reply, "Snapshots not supported on device") != NULL) { + } else if (strstr(reply, "Snapshots not supported on device")) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", reply); goto cleanup; - } else if (strstr(reply, "Error") != NULL - && strstr(reply, "while deleting snapshot") != NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", reply); + } else if (strstr(reply, "Error") && + strstr(reply, "while deleting snapshot")) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Failed to delete snapshot: %s"), reply); goto cleanup; } -- 2.21.0
qemu 4.0.0 will prefix most errors with 'Error: ', so consider any string instance of that an error. This fixes savevm failure detection when migration is blocked due to usage of nested VMX https://bugzilla.redhat.com/show_bug.cgi?id=1697997 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_monitor_text.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 76095674ab..280cc58840 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -182,6 +182,7 @@ qemuMonitorTextCreateSnapshot(qemuMonitorPtr mon, if (strstr(reply, "Error while creating snapshot") || strstr(reply, "Could not open VM state file") || strstr(reply, "State blocked by non-migratable device") || + strstr(reply, "Error: ") || (strstr(reply, "Error") && strstr(reply, "while writing VM"))) { virReportError(VIR_ERR_OPERATION_FAILED, _("Failed to take snapshot: %s"), reply); @@ -229,6 +230,7 @@ int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const char *name) _("Failed to load snapshot: %s"), reply); goto cleanup; } else if (strstr(reply, "Could not open VM state file") || + strstr(reply, "Error: ") || (strstr(reply, "Error") && (strstr(reply, "while loading VM state") || strstr(reply, "while activating snapshot on")))) { @@ -266,8 +268,9 @@ int qemuMonitorTextDeleteSnapshot(qemuMonitorPtr mon, const char *name) } else if (strstr(reply, "Snapshots not supported on device")) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", reply); goto cleanup; - } else if (strstr(reply, "Error") && - strstr(reply, "while deleting snapshot")) { + } else if (strstr(reply, "Error: ") || + (strstr(reply, "Error") && + strstr(reply, "while deleting snapshot"))) { virReportError(VIR_ERR_OPERATION_FAILED, _("Failed to delete snapshot: %s"), reply); goto cleanup; -- 2.21.0
On 4/10/19 8:29 PM, Cole Robinson wrote:
qemu in Fedora 30 rejects migration if the VM has nested VMX configured. This means managedsave and snapshots are rejected too. Unfortunately the scraping we need to resort to with the text monitor snapshot commands is not detecting that 'savevm' actually failed, so the snapshot appears to succeed.
https://bugzilla.redhat.com/show_bug.cgi?id=1697997
qemu 4.0.0 will add an 'Error: ' prefix to most hmp command errors (qemu commit 66363e9a43f), let's use that to detect error as well
Cole Robinson (3): qemu: monitor: cleanup loadvm error handling qemu: monitor cleanup delvm error handling qemu: monitor: check for common 'Error: ' string
src/qemu/qemu_monitor_text.c | 39 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-)
ACK Michal
participants (2)
-
Cole Robinson -
Michal Privoznik