[libvirt] [PATCH] Return error when updating cdrom device

[Pavel Hrdina]

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 at 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