8:46 a.m.
On 07/03/2015 09:51 AM, Pavel Hrdina wrote:
On Wed, Jul 01, 2015 at 05:39:49PM -0400, John Ferlan wrote:
>
>
> On 06/29/2015 11:17 AM, Pavel Hrdina wrote:
>> Some guests lock the tray and QEMU eject command will simply fail to
>> eject the media. But the guest OS can handle this attempt to eject the
>> media and can unlock the tray and open it. In this case, we should try
>> again to actually eject the media.
>>
>> If the first attempt fails to detect a tray_open we will fail with
>> error, from monitor. If we receive that event, we know, that the guest
>> properly reacted to the eject request, unlocked the tray and opened it.
>> In this case, we need to run the command again to actually eject the
>> media from the device. The reason to call it again is, that QEMU don't
s/don't/doesn't
>> wait for the guest to react and report an error, that the
tray is
>> locked.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1147471
>>
>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
>> ---
>> src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++--------------------------
>> src/qemu/qemu_process.c | 2 ++
>> 2 files changed, 36 insertions(+), 39 deletions(-)
>>
I suppose if I had gone back to the commit message and reread it in
light of what I was reading in the qemu-devel message linked in the bz
and what I saw in the code, then perhaps it would all make sense. Of
course the text you add below now makes it clear what the order of
processing is and why the while loop is used.
Personally I think this is a case where that commit message could be
placed into the code to make it "clearer" what is being done and why. I
know there are those against that because it's in the commit message or
perhaps some linked bug, but if I'm reading code and wondering what it
does, I'm not necessarily going back to the commit/bug that added the
code (OK, at least not at first).
Whether you add a comment into the code to indicate why this while loop
is important and how the processing works due to the event and locked
media...
ACK series,
John
>> diff --git a/src/qemu/qemu_hotplug.c
b/src/qemu/qemu_hotplug.c
>> index 0628964..17595b7 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -59,7 +59,7 @@
>>
>> VIR_LOG_INIT("qemu.qemu_hotplug");
>>
>> -#define CHANGE_MEDIA_RETRIES 10
>> +#define CHANGE_MEDIA_TIMEOUT 5000
>>
>> /* Wait up to 5 seconds for device removal to finish. */
>> unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
>> @@ -166,12 +166,13 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>> virStorageSourcePtr newsrc,
>> bool force)
>> {
>> - int ret = -1;
>> + int ret = -1, rc;
>> char *driveAlias = NULL;
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> - int retries = CHANGE_MEDIA_RETRIES;
>> const char *format = NULL;
>> char *sourcestr = NULL;
>> + bool ejectRetry = false;
>> + unsigned long long now;
>>
>> if (!disk->info.alias) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -193,36 +194,31 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>> if (!(driveAlias = qemuDeviceDriveHostAlias(disk, priv->qemuCaps)))
>> goto error;
>>
>> - qemuDomainObjEnterMonitor(driver, vm);
>> - ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
>> - if (qemuDomainObjExitMonitor(driver, vm) < 0) {
>> - ret = -1;
>> - goto cleanup;
>> - }
>> -
>> - if (ret < 0)
>> - goto error;
>> + do {
>> + qemuDomainObjEnterMonitor(driver, vm);
>> + rc = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
>> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> + goto cleanup;
>>
>> - virObjectRef(vm);
>> - /* we don't want to report errors from media tray_open polling */
>> - while (retries) {
>> - if (disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)
>> - break;
>> + if (rc == -2) {
>> + /* we've already tried, error out */
>> + if (ejectRetry)
>> + goto error;
>> + VIR_DEBUG("tray is locked, wait for the guest to unlock "
>> + "the tray and try to eject it again");
>> + ejectRetry = true;
>> + } else if (rc < 0) {
>> + goto error;
>> + }
>>
>> - retries--;
>> - virObjectUnlock(vm);
>> - VIR_DEBUG("Waiting 500ms for tray to open. Retries left %d",
retries);
>> - usleep(500 * 1000); /* sleep 500ms */
>> - virObjectLock(vm);
>> - }
>> - virObjectUnref(vm);
>> + if (virTimeMillisNow(&now) < 0)
>> + goto error;
>>
>> - if (retries <= 0) {
>> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> - _("Unable to eject media"));
>> - ret = -1;
>> - goto error;
>> - }
>> + while (disk->tray_status != VIR_DOMAIN_DISK_TRAY_OPEN) {
>> + if (virDomainObjWaitUntil(vm, now + CHANGE_MEDIA_TIMEOUT) != 0)
>> + goto error;
>> + }
>
> Seems this should be
>
> if (rc == -2) wait for TRAY_MOVED (need a new event)
> else wait for TRAY_OPEN
>
There is no event TRAY_OPEN or TRAY_CLOSE. QEMU has only one event,
DEVICE_TRAY_MOVED. This event is handled by qemuMonitorJSONHandleTrayChange,
which will set the reason to OPEN/CLOSE and emit a qemu monitor event. For this
qemu monitor event there is a handler called qemuProcessHandleTrayChange, where
the tray_status is set to VIR_DOMAIN_DISK_TRAY_OPEN or
VIR_DOMAIN_DISK_TRAY_CLOSED. This handler also wakes up that condition we are
waiting in this code. There is no need for a new event. We are already using
everything, that is available.
Pavel