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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list