[libvirt] [PATCH] Return error when updating cdrom device
The commit 84c59ffa improved the way we change ejectable media. If for any reason the first "eject" didn't open the tray we should return with error. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 125a2db..47ec779 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -125,6 +125,7 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* If ret == -1, EjectMedia already set an error message */ virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Unable to eject media")); + ret = -1; } goto audit; } -- 1.8.5.5
On 05/19/14 16:07, Pavel Hrdina wrote:
The commit 84c59ffa improved the way we change ejectable media. If for any reason the first "eject" didn't open the tray we should return with error.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 125a2db..47ec779 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -125,6 +125,7 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* If ret == -1, EjectMedia already set an error message */ virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Unable to eject media")); + ret = -1;
Although this is technically correct, the control flow in this function seems a little bit weird. The eject media error should be checked before the polling loop and removed from here. Then also the ret=0 asignment a few lines below isn't useful.
} goto audit; }
I'd go with the following patch: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 125a2db..76e289b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -106,6 +106,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force); qemuDomainObjExitMonitor(driver, vm); + if (ret < 0) + goto audit; + virObjectRef(vm); /* we don't want to report errors from media tray_open polling */ while (retries) { @@ -121,14 +124,11 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virObjectUnref(vm); 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")); - } + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Unable to eject media")); + ret = -1; goto audit; } - ret = 0; src = virDomainDiskGetSource(disk); if (src) { ACK if you go with the above. Peter
On 19.5.2014 18:40, Peter Krempa wrote:
On 05/19/14 16:07, Pavel Hrdina wrote:
The commit 84c59ffa improved the way we change ejectable media. If for any reason the first "eject" didn't open the tray we should return with error.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 125a2db..47ec779 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -125,6 +125,7 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* If ret == -1, EjectMedia already set an error message */ virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Unable to eject media")); + ret = -1;
Although this is technically correct, the control flow in this function seems a little bit weird. The eject media error should be checked before the polling loop and removed from here. Then also the ret=0 asignment a few lines below isn't useful.
} goto audit; }
I'd go with the following patch:
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 125a2db..76e289b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -106,6 +106,9 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force); qemuDomainObjExitMonitor(driver, vm);
+ if (ret < 0) + goto audit; + virObjectRef(vm); /* we don't want to report errors from media tray_open polling */ while (retries) { @@ -121,14 +124,11 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, virObjectUnref(vm);
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")); - } + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Unable to eject media")); + ret = -1; goto audit; } - ret = 0;
src = virDomainDiskGetSource(disk); if (src) {
ACK if you go with the above.
Peter
Thanks, pushed with the change above, Pavel
On 05/19/2014 08:07 AM, Pavel Hrdina wrote:
The commit 84c59ffa improved the way we change ejectable media. If for any reason the first "eject" didn't open the tray we should return with error.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+)
ACK
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 125a2db..47ec779 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -125,6 +125,7 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, /* If ret == -1, EjectMedia already set an error message */ virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Unable to eject media")); + ret = -1; } goto audit; }
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Pavel Hrdina -
Peter Krempa