[libvirt] [PATCH] qemu-hotplug: fix eject media
QEMU changed the error message to: "Tray of device 'drive-sata0-0-1' is not open" and they may change the error massage in the future. This updates the code to not depend on the text from the error message but only on error itself. One more improvement is that we will store the original error in case if it's not about tray is not open. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 17 +++++++++-------- src/qemu/qemu_monitor_json.c | 12 +----------- src/qemu/qemu_monitor_text.c | 5 +---- 3 files changed, 11 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b580283..8122367 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -173,8 +173,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; const char *format = NULL; char *sourcestr = NULL; - bool ejectRetry = false; unsigned long long now; + virErrorPtr ejectError = NULL; if (!disk->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -202,15 +202,16 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if (rc == -2) { + if (rc < 0) { /* we've already tried, error out */ - if (ejectRetry) + if (ejectError) { + virSetError(ejectError); + virFreeError(ejectError); goto error; - VIR_DEBUG("tray is locked, wait for the guest to unlock " + } + ejectError = virSaveLastError(); + VIR_DEBUG("tray may be locked, wait for the guest to unlock " "the tray and try to eject it again"); - ejectRetry = true; - } else if (rc < 0) { - goto error; } if (virTimeMillisNow(&now) < 0) @@ -220,7 +221,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0) goto error; } - } while (ejectRetry && rc != 0); + } while (rc < 0); if (!virStorageSourceIsEmpty(newsrc)) { if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8352e53..66a2922 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2272,8 +2272,7 @@ int qemuMonitorJSONSetCPU(qemuMonitorPtr mon, * Run QMP command to eject a media from ejectable device. * * Returns: - * -2 on error, when the tray is locked - * -1 on all other errors + * -1 on error * 0 on success */ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, @@ -2294,15 +2293,6 @@ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, if (ret == 0) ret = qemuMonitorJSONCheckError(cmd, reply); - if (ret < 0) { - virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); - if (error) { - const char *errorStr = virJSONValueObjectGetString(error, "desc"); - if (errorStr && c_strcasestr(errorStr, "is locked")) - ret = -2; - } - } - virJSONValueFree(cmd); virJSONValueFree(reply); return ret; diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index bb87397..3129427 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1170,8 +1170,7 @@ int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, bool online) * Run HMP command to eject a media from ejectable device. * * Returns: - * -2 on error, when the tray is locked - * -1 on all other errors + * -1 on error * 0 on success */ int qemuMonitorTextEjectMedia(qemuMonitorPtr mon, @@ -1192,8 +1191,6 @@ int qemuMonitorTextEjectMedia(qemuMonitorPtr mon, * device not found, device is locked ... * No message is printed on success it seems */ if (c_strcasestr(reply, "device ")) { - if (c_strcasestr(reply, "is locked")) - ret = -2; virReportError(VIR_ERR_OPERATION_FAILED, _("could not eject media on %s: %s"), dev_name, reply); goto cleanup; -- 2.7.3
On Thu, Mar 17, 2016 at 16:50:36 +0100, Pavel Hrdina wrote:
QEMU changed the error message to:
"Tray of device 'drive-sata0-0-1' is not open"
and they may change the error massage in the future.
This updates the code to not depend on the text from the error message but only on error itself. One more improvement is that we will store the original error in case if it's not about tray is not open.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 17 +++++++++-------- src/qemu/qemu_monitor_json.c | 12 +----------- src/qemu/qemu_monitor_text.c | 5 +---- 3 files changed, 11 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b580283..8122367 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c
[...]
@@ -202,15 +202,16 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
- if (rc == -2) { + if (rc < 0) { /* we've already tried, error out */ - if (ejectRetry) + if (ejectError) { + virSetError(ejectError); + virFreeError(ejectError);
I don't think that it's necessary to store the first error message. Previously we'd report the last error due to the fact that the JSON code was setting it all the time.
goto error; - VIR_DEBUG("tray is locked, wait for the guest to unlock " + } + ejectError = virSaveLastError(); + VIR_DEBUG("tray may be locked, wait for the guest to unlock " "the tray and try to eject it again"); - ejectRetry = true; - } else if (rc < 0) { - goto error; }
if (virTimeMillisNow(&now) < 0) @@ -220,7 +221,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0) goto error; } - } while (ejectRetry && rc != 0); + } while (rc < 0);
if (!virStorageSourceIsEmpty(newsrc)) { if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8352e53..66a2922 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c
[...]
@@ -2294,15 +2293,6 @@ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, if (ret == 0) ret = qemuMonitorJSONCheckError(cmd, reply);
Additionally I think we should not even report the error from the first call to this function. Doing that will probably require us to keep the -2 return value but you'll need to add a bool argument that will suppress any virReportError in case when the qemu command failed and return -2 in that case.
- if (ret < 0) { - virJSONValuePtr error = virJSONValueObjectGet(reply, "error");
Peter
On Tue, Mar 22, 2016 at 10:18:22AM +0100, Peter Krempa wrote:
On Thu, Mar 17, 2016 at 16:50:36 +0100, Pavel Hrdina wrote:
QEMU changed the error message to:
"Tray of device 'drive-sata0-0-1' is not open"
and they may change the error massage in the future.
This updates the code to not depend on the text from the error message but only on error itself. One more improvement is that we will store the original error in case if it's not about tray is not open.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 17 +++++++++-------- src/qemu/qemu_monitor_json.c | 12 +----------- src/qemu/qemu_monitor_text.c | 5 +---- 3 files changed, 11 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b580283..8122367 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c
[...]
@@ -202,15 +202,16 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
- if (rc == -2) { + if (rc < 0) { /* we've already tried, error out */ - if (ejectRetry) + if (ejectError) { + virSetError(ejectError); + virFreeError(ejectError);
I don't think that it's necessary to store the first error message. Previously we'd report the last error due to the fact that the JSON code was setting it all the time.
Sure, I'll remove it.
goto error; - VIR_DEBUG("tray is locked, wait for the guest to unlock " + } + ejectError = virSaveLastError(); + VIR_DEBUG("tray may be locked, wait for the guest to unlock " "the tray and try to eject it again"); - ejectRetry = true; - } else if (rc < 0) { - goto error; }
if (virTimeMillisNow(&now) < 0) @@ -220,7 +221,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0) goto error; } - } while (ejectRetry && rc != 0); + } while (rc < 0);
if (!virStorageSourceIsEmpty(newsrc)) { if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8352e53..66a2922 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c
[...]
@@ -2294,15 +2293,6 @@ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon, if (ret == 0) ret = qemuMonitorJSONCheckError(cmd, reply);
Additionally I think we should not even report the error from the first call to this function. Doing that will probably require us to keep the -2 return value but you'll need to add a bool argument that will suppress any virReportError in case when the qemu command failed and return -2 in that case.
Well, that's true, we shouldn't report that error, but we already do and this patch fixes the eject media bug. This is for different patch. Pavel
participants (2)
-
Pavel Hrdina -
Peter Krempa