From: Long YunJian <long.yunjian@zte.com.cn>

We detach windows disk with libvirt-python api dom.detachDeviceFlags(xmlstr,3),
but files in windows disk is opened and busy, and libvirt return success.
We found disk not detached actually. so, we close files those opened in windows,
and want to detach again. However, we failed with device not found but disk is
exist actually.

first time detach disk:
a. get vmdef from domain->newDef here:
qemuDomainDetachDeviceLiveAndConfig
  virDomainObjCopyPersistentDef
    virDomainObjGetPersistentDef
      domain->newDef

b. delete disk config here:
qemuDomainDetachDeviceLiveAndConfig
  qemuDomainDetachDeviceConfig
    virDomainDiskRemoveByName
      virDomainDiskRemove

c. update from vmdef to newDef
qemuDomainDetachDeviceLiveAndConfig
  virDomainObjAssignDef
    domain->newDef = vmdef

second time detach disk:
get vmdef from domain->newDef here:
qemuDomainDetachDeviceLiveAndConfig
  virDomainObjCopyPersistentDef
    virDomainObjGetPersistentDef
      domain->newDef

and then,  report device not found here for disk config deleted at the first time:
qemuDomainDetachDeviceLiveAndConfig
  qemuDomainDetachDeviceConfig
    virDomainDiskRemoveByName

to fix this problem, let detach disk again, we add disk to config if timeout,
and let it deleted in processDeviceDeletedEvent if guestos detach disk successfinnaly.

Signed-off-by: Long YunJian <long.yunjian@zte.com.cn>
---
 src/qemu/qemu_driver.c  | 35 +++++++++++++++++++++++++++++++++++
 src/qemu/qemu_hotplug.c | 12 +++++++++++-
 tests/qemuhotplugtest.c |  2 +-
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d01f788aea..0ab661251c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3598,6 +3598,9 @@ processDeviceDeletedEvent(virQEMUDriver *driver,
                           const char *devAlias)
 {
     virDomainDeviceDef dev;
+    virDomainDiskDef *det_disk = NULL;
+    g_autofree char *disk_dst_name = NULL;
+    bool devicedisk = false;

     VIR_DEBUG("Removing device %s from domain %p %s",
               devAlias, vm, vm->def->name);
@@ -3616,8 +3619,19 @@ processDeviceDeletedEvent(virQEMUDriver *driver,
         if (virDomainDefFindDevice(vm->def, devAlias, &dev, true) < 0)
             goto endjob;

+        if (dev.type & VIR_DOMAIN_DEVICE_DISK) {
+            devicedisk = true;
+            disk_dst_name = g_strdup(dev.data.disk->dst);
+        }
+
         if (qemuDomainRemoveDevice(driver, vm, &dev) < 0)
             goto endjob;
+
+        if (devicedisk && disk_dst_name) {
+            if ((det_disk = virDomainDiskRemoveByName(vm->newDef, disk_dst_name))) {
+               virDomainDiskDefFree(det_disk);
+            }
+        }
     }

     qemuDomainSaveStatus(vm);
@@ -7491,6 +7505,7 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,
     g_autoptr(virDomainDeviceDef) dev_live = NULL;
     unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
     g_autoptr(virDomainDef) vmdef = NULL;
+    bool timeout = false;

     virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
                   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -7530,6 +7545,10 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,

         if ((rc = qemuDomainDetachDeviceLive(vm, dev_live, driver, false)) < 0)
             return -1;
+        if (rc == 2) {
+            timeout = true;
+            rc = 0;
+        }

         if (rc == 0 && qemuDomainUpdateDeviceList(vm, VIR_ASYNC_JOB_NONE) < 0)
             return -1;
@@ -7542,6 +7561,22 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriver *driver,
         if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0)
             return -1;

+        /*  if timeout, add device to vmdef, and delete at processDeviceDeletedEvent */
+        if (timeout && (dev_config->type & VIR_DOMAIN_DEVICE_DISK)) {
+            g_autoptr(virDomainDeviceDef) devConf = NULL;
+            unsigned int attach_parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+                                              VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
+
+            if (!(devConf = virDomainDeviceDefParse(xml, vmdef,
+                                                    driver->xmlopt, priv->qemuCaps,
+                                                    attach_parse_flags)))
+                return -1;
+            if (qemuDomainAttachDeviceConfig(vmdef, devConf, priv->qemuCaps,
+                                             attach_parse_flags,
+                                             driver->xmlopt) < 0)
+                return -1;
+        }
+
         virDomainObjAssignDef(vm, &vmdef, false, NULL);

         /* Event sending if persistent config has changed */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 774962b0df..98b47bfa9c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6090,6 +6090,13 @@ qemuDomainDetachDeviceLease(virQEMUDriver *driver,
 }


+/* Returns:
+ *  -1 Unplug of the device failed
+ *
+ *   0 Unplug of the device success
+ *
+ *   2 removal of the device did not finish in qemuDomainRemoveDeviceWaitTime
+ */
 int
 qemuDomainDetachDeviceLive(virDomainObj *vm,
                            virDomainDeviceDef *match,
@@ -6297,7 +6304,10 @@ qemuDomainDetachDeviceLive(virDomainObj *vm,
     if (async) {
         ret = 0;
     } else {
-        if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
+        ret = qemuDomainWaitForDeviceRemoval(vm);
+        if (ret == 0)
+            ret = 2;
+        if (ret == 1)
             ret = qemuDomainRemoveDevice(driver, vm, &detach);
     }

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 961a7a3c64..6c1c6112d9 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -238,7 +238,7 @@ testQemuHotplug(const void *data)

     case DETACH:
         ret = qemuDomainDetachDeviceLive(vm, dev, &driver, false);
-        if (ret == 0 || fail)
+        if (ret == 2 || ret == 0 || fail)
             ret = testQemuHotplugCheckResult(vm, domain_xml,
                                              domain_filename, fail);
         break;
--
2.31.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Fri, Apr 26, 2024 at 12:29:05 +0800, long.yunjian@zte.com.cn wrote:
> From: Long YunJian <long.yunjian@zte.com.cn>
> 
> We detach windows disk with libvirt-python api dom.detachDeviceFlags(xmlstr,3),
> but files in windows disk is opened and busy, and libvirt return success.
> We found disk not detached actually.

Note that this is expected and documented behaviour as device unplug
requires cooperation with the guest OS:

   Beware that depending on the hypervisor and device type, detaching a
   device from a running domain may be asynchronous. That is, calling
   virDomainDetachDeviceFlags may just request device removal while the
   device is actually removed later (in cooperation with a guest OS).
   Previously, this fact was ignored and the device could have been
   removed from domain configuration before it was actually removed by
   the hypervisor causing various failures on subsequent operations. To
   check whether the device was successfully removed, either recheck
   domain configuration using virDomainGetXMLDesc() or add a handler for
   the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event. In case the device is
   already gone when virDomainDetachDeviceFlags returns, the event is
   delivered before this API call ends. To help existing clients work
   better in most cases, this API will try to transform an asynchronous
   device removal that finishes shortly after the request into a
   synchronous removal. In other words, this API may wait a bit for the
   removal to complete in case it was not synchronous.

https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceFlags


> so, we close files those opened in windows,
> and want to detach again. However, we failed with device not found but disk is
> exist actually.

Okay, so reading the code reveals the real problem. You are using _LIVE
and _CONFIG flags together, in which case the persistent definition is
already removed, thus produces failure.

In case your application cares about these advanced scenarios you'll
need to handle the update of the presistent definition separately from
the update of _CONFIG in the handler for
VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED, or implement the retry without the
_CONFIG flag if it fails.

Reallistically due to backwards compatibility reasons we can't afford to
change the behaviour of the device removal in this case.

In case you'd be still wanting to get a more sane behaviour of
virDomainDetachDeviceFlags which removes the persistent (_CONFIG)
definition only after the _LIVE is detached this mode would require a
new flag to initiate it.

The new mode would then need to record which definition to remove as the
actual removal of the _LIVE device can happen at any point in the
future, and even can be fully ignored. This also means that the
information needed to remove the _CONFIG bit would need to be recorded
to the status XML to persist it across libvirtd/virtqemud restarts.

Also this would need to be implemented for all devices and not just
disks to be an acceptable patch as well as not actually atempt to remove
the device and then return it back reconstructed from the XML the user
passed as that may be incomplete.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org