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

[Bjoern Walk]

John Ferlan <jferlan at redhat.com> [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 at linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>> ---
>>  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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 896 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160721/c54b36df/attachment-0001.sig>