Re: [libvirt] [PATCH v2 4/4] qemu_hotplug: try harder to eject media

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(a)redhat.com&gt; > --- > 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

> + } 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); >