Re: [PATCH 4/7] qemu: hotplug: Add support for signalling device unplug failure

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

 



On 04/05/2016 11:09 AM, Peter Krempa wrote:
> Similarly to the DEVICE_DELETED event we will be able to tell when
> unplug of certain device types will be rejected by the guest OS. Wire up
> the device deletion signalling code to allow handling this.
> ---
>  src/qemu/qemu_domain.h  | 17 ++++++++++++++++-
>  src/qemu/qemu_hotplug.c | 30 +++++++++++++++++++++---------
>  src/qemu/qemu_hotplug.h |  3 ++-
>  src/qemu/qemu_process.c |  2 +-
>  4 files changed, 40 insertions(+), 12 deletions(-)
> 

There's a slight conflict here with git master but it should rebase cleanly,
git am --3way couldn't handle it for me for whatever reason

> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 2b92a90..84b4b16 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -148,6 +148,20 @@ struct qemuDomainJobObj {
>  typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver,
>                                            virDomainObjPtr vm);
> 
> +/* helper data types for async device unplug */
> +typedef enum {
> +    QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_NONE = 0,
> +    QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_OK,
> +    QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_GUEST_REJECTED,
> +} qemuDomainUnplugDeviceStatus;
> +
> +typedef struct _qemuDomainUnplugDevice qemuDomainUnplugDevice;
> +typedef qemuDomainUnplugDevice *qemuDomainUnplugDevicePtr;
> +struct _qemuDomainUnplugDevice {
> +    const char *alias;
> +    qemuDomainUnplugDeviceStatus status;
> +};
> +
>  typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
>  typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
>  struct _qemuDomainObjPrivate {
> @@ -198,7 +212,8 @@ struct _qemuDomainObjPrivate {
> 
>      virPerfPtr perf;
> 
> -    const char *unpluggingDevice; /* alias of the device that is being unplugged */
> +    qemuDomainUnplugDevice unplug;
> +

FWIW in isolation I'd think 'qemuDomainUnplugDevice' was a function name...
maybe stick with the UnpluggingDevice name. Not a NACK, just an idea if you're
motivated. Also lack Ptr use just looks weird, I'm so used to everything being
a Ptr in libvirt code :)

>      char **qemuDevices; /* NULL-terminated list of devices aliases known to QEMU */
> 
>      bool hookRun;  /* true if there was a hook run over this domain */
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 134f458..bd0599f 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3337,20 +3337,24 @@ qemuDomainMarkDeviceForRemoval(virDomainObjPtr vm,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> 
> -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
> -        priv->unpluggingDevice = info->alias;
> -    else
> -        priv->unpluggingDevice = NULL;
> +    memset(&priv->unplug, 0, sizeof(priv->unplug));
> +
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
> +        return;
> +
> +    priv->unplug.alias = info->alias;
>  }
> 
>  static void
>  qemuDomainResetDeviceRemoval(virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    priv->unpluggingDevice = NULL;
> +    priv->unplug.alias = NULL;
>  }
> 
>  /* Returns:
> + *  -1 Unplug of the device failed
> + *
>   *   0 DEVICE_DELETED event is supported and removal of the device did not
>   *     finish in qemuDomainRemoveDeviceWaitTime
>   *
> @@ -3373,17 +3377,23 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm)
>          return 1;
>      until += qemuDomainRemoveDeviceWaitTime;
> 
> -    while (priv->unpluggingDevice) {
> +    while (priv->unplug.alias) {
>          if ((rc = virDomainObjWaitUntil(vm, until)) == 1)
>              return 0;
> 
>          if (rc < 0) {
>              VIR_WARN("Failed to wait on unplug condition for domain '%s' "
> -                     "device '%s'", vm->def->name, priv->unpluggingDevice);
> +                     "device '%s'", vm->def->name, priv->unplug.alias);
>              return 1;
>          }
>      }
> 
> +    if (priv->unplug.status == QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_GUEST_REJECTED) {
> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                       _("unplug of device was rejected by the guest"));
> +        return -1;
> +    }
> +
>      return 1;
>  }
> 

Okay, so 0 == didn't succeed in time, 1 == definitely succeeded OR we have no
way of knowing so assume it worked, -1 == definitely failed. That's a lot of
semantics :) But this makes sense, now when the -1 trickles up the stack, it's
when we know for certain that the hotplug failed.

ACK

- Cole

> @@ -3395,12 +3405,14 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm)
>   */
>  bool
>  qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
> -                              const char *devAlias)
> +                              const char *devAlias,
> +                              qemuDomainUnplugDeviceStatus status)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> 
> -    if (STREQ_NULLABLE(priv->unpluggingDevice, devAlias)) {
> +    if (STREQ_NULLABLE(priv->unplug.alias, devAlias)) {
>          qemuDomainResetDeviceRemoval(vm);
> +        priv->unplug.status = status;
>          virDomainObjBroadcast(vm);
>          return true;
>      }
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 4140da3..e6833f0 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -122,6 +122,7 @@ int qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>                             virDomainDeviceDefPtr dev);
> 
>  bool qemuDomainSignalDeviceRemoval(virDomainObjPtr vm,
> -                                   const char *devAlias);
> +                                   const char *devAlias,
> +                                   qemuDomainUnplugDeviceStatus status);
> 
>  #endif /* __QEMU_HOTPLUG_H__ */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d9dca74..bbde32c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1359,7 +1359,7 @@ qemuProcessHandleDeviceDeleted(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>      VIR_DEBUG("Device %s removed from domain %p %s",
>                devAlias, vm, vm->def->name);
> 
> -    if (qemuDomainSignalDeviceRemoval(vm, devAlias))
> +    if (qemuDomainSignalDeviceRemoval(vm, devAlias, QEMU_DOMAIN_UNPLUG_DEVICE_STATUS_OK))
>          goto cleanup;
> 
>      if (VIR_ALLOC(processEvent) < 0)
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]