Re: [PATCH] Return error when updating cdrom device

[Peter Krempa <pkrempa@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list