[libvirt] [PATCH v2 01/11] qemu: track hostdev delete intention

Daniel Henrique Barboza danielhb413 at gmail.com
Thu Sep 12 12:24:16 UTC 2019


I had to amend the patch to apply it to the current master (commit
e9d51a221c). git am was putting the 'if (!unplug)' block in the
wrong function and the code didn't compile. Here's what I did:

-------

$ git diff
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 96b9ff998d..08ad8ef8cc 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5458,20 +5458,6 @@ qemuDomainDetachPrepController(virDomainObjPtr vm,
          return -1;
      }

-    /*
-     * Why having additional check in second branch? Suppose client
-     * asks for device detaching and we delete device from qemu
-     * but don't get DEVICE_DELETED event yet. Next USB is unplugged
-     * from host and we have this function called again. If we reset
-     * delete action to 'unplug' then device will be left in
-     * libvirt config after handling DEVICE_DELETED event while
-     * it should not as client asked to detach the device before.
-     */
-     */
-    if (!unplug)
-        hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE;
-    else if (hostdev->deleteAction != 
VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE)
-        hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG;
-
      return 0;
  }

@@ -5553,6 +5539,20 @@ qemuDomainDetachPrepHostdev(virDomainObjPtr vm,
          return -1;
      }

+    /*
+     * Why having additional check in second branch? Suppose client
+     * asks for device detaching and we delete device from qemu
+     * but don't get DEVICE_DELETED event yet. Next USB is unplugged
+     * from host and we have this function called again. If we reset
+     * delete action to 'unplug' then device will be left in
+     * libvirt config after handling DEVICE_DELETED event while
+     * it should not as client asked to detach the device before.
+     */
+    if (!unplug)
+        hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE;
+    else if (hostdev->deleteAction != 
VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE)
+        hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG;
+
      return 0;
  }

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 3c177c6622..4e7851aa62 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -158,7 +158,7 @@ testQemuHotplugDetach(virDomainObjPtr vm,
      case VIR_DOMAIN_DEVICE_SHMEM:
      case VIR_DOMAIN_DEVICE_WATCHDOG:
      case VIR_DOMAIN_DEVICE_HOSTDEV:
-        ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async);
+        ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async, false);
          break;
      default:
          VIR_TEST_VERBOSE("device type '%s' cannot be detached",

-------


A few more nits below:


On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:
> We are going to call qemuDomainDetachDeviceLive when usb device
> is unplugged from host. But later when DEVICE_DELETED event is delivered
> we need to keep device in libvirt config. For this purpuse let's save


s/purpuse/purpose

> delete intention in device config.

There's an explanation of why are you making this change in
the comment block you put in qemuDomainDetachPrepHostdev
(and across the other commit messages of the series). It would be good
to put more a bit more info about the motivations in this first commit
message as well (like you did in patch 06), since this patch is the backbone
of the design change you're making.


>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>   src/conf/domain_conf.h  | 14 ++++++++++++++
>   src/qemu/qemu_driver.c  |  4 ++--
>   src/qemu/qemu_hotplug.c | 23 ++++++++++++++++++++---
>   src/qemu/qemu_hotplug.h |  3 ++-
>   tests/qemuhotplugtest.c |  2 +-
>   5 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 33cef5b75c..19a5b21462 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -326,6 +326,19 @@ struct _virDomainHostdevCaps {
>       } u;
>   };
>   
> +typedef enum {
> +    VIR_DOMAIN_HOSTDEV_DELETE_ACTION_NONE = 0,
> +    /* delete associated device from libvirt config
> +     * as intended by client API call */
> +    VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE,
> +    /* keep associated device in libvirt config as
> +     * qemu device is deleted as a result of unplugging
> +     * device from host */
> +    VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG,
> +
> +    VIR_DOMAIN_HOSTDEV_DELETE_ACTION_LAST
> +} virDomainHostdevDeleteActionType;
> +
>   
>   /* basic device for direct passthrough */
>   struct _virDomainHostdevDef {
> @@ -343,6 +356,7 @@ struct _virDomainHostdevDef {
>       bool missing;
>       bool readonly;
>       bool shareable;
> +    virDomainHostdevDeleteActionType deleteAction;
>       union {
>           virDomainHostdevSubsys subsys;
>           virDomainHostdevCaps caps;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 78f5471b79..2378a2e7d0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9123,7 +9123,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver,
>       if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>           int rc;
>   
> -        if ((rc = qemuDomainDetachDeviceLive(vm, dev_copy, driver, false)) < 0)
> +        if ((rc = qemuDomainDetachDeviceLive(vm, dev_copy, driver, false, false)) < 0)
>               goto cleanup;
>   
>           if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> @@ -9212,7 +9212,7 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver,
>           if (virDomainDefFindDevice(def, alias, &dev, true) < 0)
>               goto cleanup;
>   
> -        if ((rc = qemuDomainDetachDeviceLive(vm, &dev, driver, true)) < 0)
> +        if ((rc = qemuDomainDetachDeviceLive(vm, &dev, driver, true, false)) < 0)
>               goto cleanup;
>   
>           if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 63acb9c451..559254b234 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5348,7 +5348,8 @@ qemuDomainDetachPrepController(virDomainObjPtr vm,
>   static int
>   qemuDomainDetachPrepHostdev(virDomainObjPtr vm,
>                               virDomainHostdevDefPtr match,
> -                            virDomainHostdevDefPtr *detach)
> +                            virDomainHostdevDefPtr *detach,
> +                            bool unplug)
>   {
>       virDomainHostdevSubsysPtr subsys = &match->source.subsys;
>       virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb;
> @@ -5420,6 +5421,20 @@ qemuDomainDetachPrepHostdev(virDomainObjPtr vm,
>           return -1;
>       }
>   
> +    /*
> +     * Why having additional check in second branch? Suppose client
> +     * asks for device detaching and we delete device from qemu
> +     * but don't get DEVICE_DELETED event yet. Next USB is unplugged
> +     * from host and we have this function called again. If we reset
> +     * delete action to 'unplug' then device will be left in
> +     * libvirt config after handling DEVICE_DELETED event while
> +     * it should not as client asked to detach the device before.
> +     */
> +    if (!unplug)
> +        hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE;
> +    else if (hostdev->deleteAction != VIR_DOMAIN_HOSTDEV_DELETE_ACTION_DELETE)
> +        hostdev->deleteAction = VIR_DOMAIN_HOSTDEV_DELETE_ACTION_UNPLUG;
> +
>       return 0;
>   }
>   
> @@ -5721,7 +5736,8 @@ int
>   qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>                              virDomainDeviceDefPtr match,
>                              virQEMUDriverPtr driver,
> -                           bool async)
> +                           bool async,
> +                           bool unplug)
>   {
>       virDomainDeviceDef detach = { .type = match->type };
>       virDomainDeviceInfoPtr info = NULL;
> @@ -5766,7 +5782,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>           break;
>       case VIR_DOMAIN_DEVICE_HOSTDEV:
>           if (qemuDomainDetachPrepHostdev(vm, match->data.hostdev,
> -                                        &detach.data.hostdev) < 0) {
> +                                        &detach.data.hostdev,
> +                                        unplug) < 0) {
>               return -1;
>           }
>           break;
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 896e6c7b98..f7ebf3f505 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -116,7 +116,8 @@ int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
>   int qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>                                  virDomainDeviceDefPtr match,
>                                  virQEMUDriverPtr driver,
> -                               bool async);
> +                               bool async,
> +                               bool unplug);
>   
>   void qemuDomainRemoveVcpuAlias(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm,
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> index b6aad330a9..ef91d4f131 100644
> --- a/tests/qemuhotplugtest.c
> +++ b/tests/qemuhotplugtest.c
> @@ -151,7 +151,7 @@ testQemuHotplugDetach(virDomainObjPtr vm,
>       case VIR_DOMAIN_DEVICE_CHR:
>       case VIR_DOMAIN_DEVICE_SHMEM:
>       case VIR_DOMAIN_DEVICE_WATCHDOG:
> -        ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async);
> +        ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async, false);
>           break;
>       default:
>           VIR_TEST_VERBOSE("device type '%s' cannot be detached",

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190912/062b24b8/attachment-0001.htm>


More information about the libvir-list mailing list