Re: [PATCH] qemu: hotplug: fix changeable media ejection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 07/21/2016 07:43 AM, Bjoern Walk wrote:
> John Ferlan <jferlan@xxxxxxxxxx> [2016-07-21, 12:23PM +0200]:
>>
>>
>> On 07/08/2016 06:30 AM, Bjoern Walk wrote:
>>> Since return code is checked globally at the end of the function, let's
>>> make sure that we set it correctly at any point.
>>>
>>> This fixes a regression introduced in commit 0aa19f35 where the first
>>> command to eject changeable media would fail unconditionally.
>>>
>>> Signed-off-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
>>> ---
>>>  src/qemu/qemu_hotplug.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index c322543..789f18c 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -240,7 +240,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr
>>> driver,
>>>      /* If the tray is present and tray change event is supported
>>> wait for it to open. */
>>>      if (diskPriv->tray &&
>>>          virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
>>> -        if (qemuHotplugWaitForTrayEject(driver, vm, disk,
>>> driveAlias, force) < 0)
>>> +        rc = qemuHotplugWaitForTrayEject(driver, vm, disk,
>>> driveAlias, force);
>>> +        if (rc < 0)
>>>              goto error;
>>
>> But rc isn't checked in the error and cleanup labels in
>> qemuDomainChangeEjectableMedia, so not sure what the purpose of setting
>> rc is.  What am I missing?
>>
> 
> It takes two issues of qemuMonitorEjectMedia in order to successfully
> eject the media as far as I understood because the first only opens the
> tray only. Hence
> 
> 235     qemuDomainObjEnterMonitor(driver, vm);
> 236     rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
> 
> rc == -1 for the first call.
> 
> 237     if (qemuDomainObjExitMonitor(driver, vm) < 0)
> 238         goto cleanup;
> 239
> 240     /* If the tray is present and tray change event is supported
> wait for it to open. */
> 241     if (diskPriv->tray &&
> 242         virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_TRAY_MOVED)) {
> 243         if (qemuHotplugWaitForTrayEject(driver, vm, disk,
> driveAlias, force) < 0)
> 
> This is the second call to qemuMonitorEjectMedia after waiting an
> appropriate time to process the tray open command above. This call
> succeeds and returns 0 but the return code rc is _not_ set, so rc is
> still -1.
> 

Ahh - the success case.  I was reading as the failure and goto case
probably because of the commit message.

So ACK to the patch and I just pushed

John

> 244             goto error;
> 245     } else  {
> 246         /* otherwise report possible errors from the attempt to
> eject the media*/
> 247         if (rc < 0)
> 248             goto error;
> 249     }
> 250
> 251     if (!virStorageSourceIsEmpty(newsrc)) {
> 
> This doesn't get called for just an eject media command...
> 
> [...]
> 264         rc = qemuMonitorChangeMedia(priv->mon,
> 
> ... but it _would_ actually set rc correctly.
> 
> 265                                     driveAlias,
> 266                                     sourcestr,
> 267                                     format);
> 268         if (qemuDomainObjExitMonitor(driver, vm) < 0)
> 269             goto cleanup;
> 270     }
> 271
> 272     virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0);
> 273
> 274     if (rc < 0)
> 275         goto error;
> 
> Since rc is still -1 because it did not get set properly on each branch,
> the function errors out despite actually succeeding. This is the actual
> behaviour of this function prior to the refactoring, so I guess it's
> just a simple oversight. Unfortunately, the test suite did not catch
> this because of a missing mock in qemuhotplugtest.
> 
>> The existing "else" in the code is for (diskPriv->tray && virQEMU...)
>> condition as I see it.
>>
>> If perhaps you had just set rc, not done the goto, and remove the else
>> condition, e.g.:
>>
>>    if (diskPriv... && virQEMUCaps...)
>>        rc = qemuHotplugWait...
>>
>>    if (rc < 0)
>>        goto error;
>>
>> that's just doing the same thing in a different way.
>>
> 
> I don't understand. My patch restores the exact functional behaviour of
> this function prior to the refactor. I do see a spurious error message
> when issuing the first change-media --eject command. This is gone with
> my patch applied. If the function is otherwise misbehaving I can not
> tell.
> 
>> John
>>>      } else  {
>>>          /* otherwise report possible errors from the attempt to
>>> eject the media*/
>>>
>>
> 
> Best,
>        Bjoern

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]