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 > > 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@xxxxxxxxxx> > > --- > > src/qemu/qemu_hotplug.c | 73 +++++++++++++++++++++++-------------------------- > > src/qemu/qemu_process.c | 2 ++ > > 2 files changed, 36 insertions(+), 39 deletions(-) > > > > 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 > > > + } while (ejectRetry && rc != 0); > > > > if (!virStorageSourceIsEmpty(newsrc)) { > > if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0) > > @@ -237,19 +233,17 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, > > } > > } > > qemuDomainObjEnterMonitor(driver, vm); > > - ret = qemuMonitorChangeMedia(priv->mon, > > - driveAlias, > > - sourcestr, > > - format); > > - if (qemuDomainObjExitMonitor(driver, vm) < 0) { > > - ret = -1; > > + rc = qemuMonitorChangeMedia(priv->mon, > > + driveAlias, > > + sourcestr, > > + format); > > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > > goto cleanup; > > - } > > } > > > > - virDomainAuditDisk(vm, disk->src, newsrc, "update", ret >= 0); > > + virDomainAuditDisk(vm, disk->src, newsrc, "update", rc >= 0); > > > > - if (ret < 0) > > + if (rc < 0) > > goto error; > > > > /* remove the old source from shared device list */ > > @@ -259,6 +253,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, > > virStorageSourceFree(disk->src); > > disk->src = newsrc; > > newsrc = NULL; > > + ret = 0; > > > > cleanup: > > VIR_FREE(driveAlias); > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > > index a688feb..648ba00 100644 > > --- a/src/qemu/qemu_process.c > > +++ b/src/qemu/qemu_process.c > > @@ -1152,6 +1152,8 @@ qemuProcessHandleTrayChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED, > > VIR_WARN("Unable to save status on vm %s after tray moved event", > > vm->def->name); > > } > > Seems there should be some sort of DEVICE_TRAY_MOVED event... as well as > OPEN and CLOSE > > John > > > + > > + virDomainObjBroadcast(vm); > > } > > > > virObjectUnlock(vm); > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list