[libvirt] [PATCH v2 7/8] qemu: Remove devices only after DEVICE_DELETED event

[Daniel P. Berrange]

On Thu, Jul 18, 2013 at 12:03:49PM +0200, Jiri Denemark wrote:
> ---
> 
> Notes:
>     Version 2:
>     - DEVICE_DELETED event handler always removes the device
>     - wait for up to 5 seconds after device_del returns to make
>       async removal synchronous in normal cases
> 
>  src/qemu/qemu_domain.c  |   4 ++
>  src/qemu/qemu_domain.h  |   3 +
>  src/qemu/qemu_hotplug.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_hotplug.h |   7 +++
>  src/qemu/qemu_process.c |  32 ++++++++++
>  5 files changed, 199 insertions(+), 6 deletions(-)

> +static void
> +qemuDomainMarkDeviceForRemoval(virDomainObjPtr vm,
> +                               virDomainDeviceInfoPtr info)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
> +        priv->unpluggingDevice = info->alias;
> +    else
> +        priv->unpluggingDevice = NULL;
> +}

Ok, this is safe because the callers have locked 'vm'.

> +static void
> +qemuDomainResetDeviceRemoval(virDomainObjPtr vm)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    priv->unpluggingDevice = NULL;
> +}

likewise here

> +
> +/* Returns:
> + *  -1 on error
> + *   0 when DEVICE_DELETED event is unsupported
> + *   1 when device removal finished
> + *   2 device removal did not finish in QEMU_REMOVAL_WAIT_TIME
> + */
> +static int
> +qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    unsigned long long until;
> +
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
> +        return 0;
> +
> +    if (virTimeMillisNow(&until) < 0)
> +        return -1;
> +    until += QEMU_REMOVAL_WAIT_TIME;
> +
> +    while (priv->unpluggingDevice) {
> +        if (virCondWaitUntil(&priv->unplugFinished,
> +                             &vm->parent.lock, until) < 0) {
> +            if (errno == ETIMEDOUT) {
> +                return 2;
> +            } else {
> +                virReportSystemError(errno, "%s",
> +                                     _("Unable to wait on unplug condition"));
> +                return -1;
> +            }
> +        }
> +    }
> +
> +    return 1;
> +}

and virCondWaitUntil is unlocking the 'vm', but this is
safe too, since we're inside a BeginJob block which holds
an extra reference.

> +static int
> +qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> +                               virDomainObjPtr vm,
> +                               const char *devAlias)
> +{
> +    virQEMUDriverPtr driver = qemu_driver;
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    virDomainDeviceDef dev;
> +
> +    virObjectLock(vm);
> +
> +    VIR_DEBUG("Device %s removed from domain %p %s",
> +              devAlias, vm, vm->def->name);
> +
> +    qemuDomainSignalDeviceRemoval(vm, devAlias);
> +
> +    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");
> +
> +cleanup:
> +    virObjectUnlock(vm);
> +    virObjectUnref(cfg);
> +    return 0;
> +}

Ok, and this holds the lock too. So this all looks correctly synchronized


ACK

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 :|