Re: [libvirt] [PATCH 11/12] qemu: Remove devices only after DEVICE_DELETED event
On Mon, Jul 15, 2013 at 18:25:58 +0100, Daniel Berrange wrote:
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.
No, actually it's storing state for only a part of the method call. It
is set before the method enters monitor and it is checked and reset when
the method returns from monitor. This is all because DEVICE_DELETED may
be delivered before the monitor command that caused it returns, in which
case we don't want the event handler to remove that device but rather
let the method itself finish its removal. Alternatively, we could remove
the device directly in DEVICE_DELETED handler everytime but we would
still need a way to notify the method that its device has already been
removed. Although we could change it into a condition and move it to the
monitor internal data structure.
...
> +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.
Yes, that would be an alternative approach.
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.
Hmm, that would work too and would probably provide a better user
experience esp. for interactive usage. In other words, we would pretend
that any device which finished unplug in 5 or 10 seconds was removed
synchronously. Sounds reasonable.
Jirka