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

Re: [libvirt] [PATCH 11/12] qemu: Remove devices only after DEVICE_DELETED event



On Mon, Jul 15, 2013 at 07:11:12PM +0200, Jiri Denemark wrote:
> ---
>  src/qemu/qemu_domain.h  |   2 +
>  src/qemu/qemu_hotplug.c | 101 +++++++++++++++++++++++++++++++++++++++++++++---
>  src/qemu/qemu_hotplug.h |   3 ++
>  src/qemu/qemu_process.c |  41 ++++++++++++++++++++
>  4 files changed, 142 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 750841f..ba273ed 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -168,6 +168,8 @@ struct _qemuDomainObjPrivate {
>      size_t ncleanupCallbacks_max;
>  
>      virCgroupPtr cgroup;
> +
> +    const char *deletingDevice; /* alias of the device that is being deleted */

This field just feels very wrong to me. It is storing state related
to a single method call, outside the execution lifetime of the method
call.

> @@ -2385,6 +2428,11 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
>                      QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0)
>          goto cleanup;
>  
> +    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
> +        priv->deletingDevice = detach->info.alias;
> +    else
> +        priv->deletingDevice = NULL;
> +
>      qemuDomainObjEnterMonitor(driver, vm);
>      if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>          if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {

Can't we get rid of this bit here...

> @@ -2406,10 +2454,12 @@ int qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
>  
>      qemuDomainObjExitMonitor(driver, vm);
>  
> -    qemuDomainRemoveDiskDevice(driver, vm, detach);
> +    if (!priv->deletingDevice)
> +        qemuDomainRemoveDiskDevice(driver, vm, detach);
>      ret = 0;

And change this to

   if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
      qemuDomainRemoveDiskDevice(driver, vm, detach);


> +static int
> +qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> +                               virDomainObjPtr vm,
> +                               const char *devAlias)
> +{
> +    virQEMUDriverPtr driver = qemu_driver;
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    qemuDomainObjPrivatePtr priv;
> +    virDomainDeviceDef dev;
> +
> +    virObjectLock(vm);
> +
> +    VIR_DEBUG("Device %s removed from domain %p %s",
> +              devAlias, vm, vm->def->name);
> +
> +    priv = vm->privateData;
> +
> +    if (STREQ_NULLABLE(priv->deletingDevice, devAlias)) {
> +        /* We got this event before the command that requested removal of
> +         * devAlias returned. Clear deletingDevice so that the unplugging
> +         * code know it has to remove the device.
> +         */
> +        priv->deletingDevice = NULL;
> +    } else {
> +        if (virDomainDefFindDevice(vm->def, devAlias, &dev) < 0)
> +            goto cleanup;
> +
> +        qemuDomainRemoveDevice(driver, vm, &dev);
> +
> +        if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> +            VIR_WARN("unable to save domain status with balloon change");
> +    }

And kill the first part of this if() block, leaving only the else(),
so removal of the device is fully delegated to this callback when the
event is known to exist.


Also, if we have the event, I think we ought to make the virDomainDetachDevice()
API block waiting for arrival of the event, for some reasonable finite period of
time.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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