[libvirt] [PATCH] qemu: Don't report error on successful media eject
If we are just ejecting media, ret == -1 even after the retry loop determines that the tray is open, as requested. This means media disconnect always report's error. Fix it, and fix some other mini issues: - Don't overwrite the 'eject' error message if the retry loop fails - Move the retries decrement inside the loop, otherwise the final loop might succeed, yet retries == 0 and we will raise error - Setting ret = -1 in the disk->src check is unneeded - Fix comment typos cc: mprivozn@redhat.com --- src/qemu/qemu_hotplug.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1ed61db..3b277ef 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -98,10 +98,11 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virObjectRef(vm); /* we don't want to report errors from media tray_open polling */ - while (retries--) { + while (retries) { if (origdisk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN) break; + retries--; virObjectUnlock(vm); VIR_DEBUG("Waiting 500ms for tray to open. Retries left %d", retries); usleep(500 * 1000); /* sleep 500ms */ @@ -109,19 +110,20 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } virObjectUnref(vm); - if (disk->src) { - /* deliberately don't depend on 'ret' as 'eject' may have failed for the - * fist time and we are gonna check the drive state anyway */ - const char *format = NULL; - - /* We haven't succeeded yet */ - ret = -1; - - if (retries <= 0) { + if (retries <= 0) { + if (ret == 0) { + /* If ret == -1, EjectMedia already set an error message */ virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Unable to eject media before changing it")); - goto audit; + _("Unable to eject media")); } + goto audit; + } + ret = 0; + + if (disk->src) { + /* deliberately don't depend on 'ret' as 'eject' may have failed the + * first time and we are gonna check the drive state anyway */ + const char *format = NULL; if (disk->type != VIR_DOMAIN_DISK_TYPE_DIR) { if (disk->format > 0) -- 1.8.2.1
On 05/28/2013 08:37 AM, Cole Robinson wrote:
If we are just ejecting media, ret == -1 even after the retry loop determines that the tray is open, as requested. This means media disconnect always report's error.
Fix it, and fix some other mini issues:
- Don't overwrite the 'eject' error message if the retry loop fails - Move the retries decrement inside the loop, otherwise the final loop might succeed, yet retries == 0 and we will raise error - Setting ret = -1 in the disk->src check is unneeded - Fix comment typos
cc: mprivozn@redhat.com --- src/qemu/qemu_hotplug.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
ACK.
+ + if (disk->src) { + /* deliberately don't depend on 'ret' as 'eject' may have failed the + * first time and we are gonna check the drive state anyway */
Comment motion, but might be worth s/gonna/going to/ while touching it (I'm actually surprised that thunderbird didn't flag the slang 'gonna' as a misspelled word). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/28/2013 11:31 AM, Eric Blake wrote:
On 05/28/2013 08:37 AM, Cole Robinson wrote:
If we are just ejecting media, ret == -1 even after the retry loop determines that the tray is open, as requested. This means media disconnect always report's error.
Fix it, and fix some other mini issues:
- Don't overwrite the 'eject' error message if the retry loop fails - Move the retries decrement inside the loop, otherwise the final loop might succeed, yet retries == 0 and we will raise error - Setting ret = -1 in the disk->src check is unneeded - Fix comment typos
cc: mprivozn@redhat.com --- src/qemu/qemu_hotplug.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
ACK.
+ + if (disk->src) { + /* deliberately don't depend on 'ret' as 'eject' may have failed the + * first time and we are gonna check the drive state anyway */
Comment motion, but might be worth s/gonna/going to/ while touching it (I'm actually surprised that thunderbird didn't flag the slang 'gonna' as a misspelled word).
Thanks, pushed with that comment fix. - Cole
participants (2)
-
Cole Robinson -
Eric Blake