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