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

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

 



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



[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]