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@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
>
>
Thanks, pushed with the change above,
Pavel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list