Disk hotplug has slightly different semantics from media changing. Move
the media change code out and add proper initialization of the new
source object.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_driver.c  | 15 +----------
 src/qemu/qemu_hotplug.c | 56 +++++++++++++++++++++++++----------------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 64f0ad33e8..f609151152 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7851,12 +7851,6 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
     virDomainDeviceDef oldDev = { .type = dev->type };
     int ret = -1;

-    if (virDomainDiskTranslateSourcePool(disk) < 0)
-        goto cleanup;
-
-    if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0)
-        goto cleanup;
-
     if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
                                                    disk->bus, disk->dst))) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -7885,16 +7879,9 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
             goto cleanup;
         }

-        /* Add the new disk src into shared disk hash table */
-        if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0)
-            goto cleanup;
-
         if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
-                                           dev->data.disk->src, force) < 0) {
-            ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk,
-                                              vm->def->name));
+                                           dev->data.disk->src, force) < 0)
             goto cleanup;
-        }

         dev->data.disk->src = NULL;
     }
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 6227a130da..ef3f0cd8b3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -737,6 +737,15 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,

     disk->src = newsrc;

+    if (virDomainDiskTranslateSourcePool(disk) < 0)
+        goto cleanup;
+
+    if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0)
+        goto cleanup;
+
+    if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0)
+        goto cleanup;
+
     if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0)
         goto cleanup;

@@ -1068,9 +1077,15 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver,
 {
     size_t i;
     virDomainDiskDefPtr disk = dev->data.disk;
-    virDomainDiskDefPtr orig_disk = NULL;
     int ret = -1;

+    if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
+        disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cdrom/floppy device hotplug isn't supported"));
+        return -1;
+    }
+
     if (virDomainDiskTranslateSourcePool(disk) < 0)
         goto cleanup;

@@ -1084,27 +1099,6 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver,
         goto cleanup;

     switch ((virDomainDiskDevice) disk->device)  {
-    case VIR_DOMAIN_DISK_DEVICE_CDROM:
-    case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
-        if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
-                                                       disk->bus, disk->dst))) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("No device with bus '%s' and target '%s'. "
-                             "cdrom and floppy device hotplug isn't supported "
-                             "by libvirt"),
-                           virDomainDiskBusTypeToString(disk->bus),
-                           disk->dst);
-            goto cleanup;
-        }
-
-        if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
-                                           disk->src, false) < 0)
-            goto cleanup;
-
-        disk->src = NULL;
-        ret = 0;
-        break;
-
     case VIR_DOMAIN_DISK_DEVICE_DISK:
     case VIR_DOMAIN_DISK_DEVICE_LUN:
         for (i = 0; i < vm->def->ndisks; i++) {
@@ -1146,6 +1140,8 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver,
         }
         break;

+    case VIR_DOMAIN_DISK_DEVICE_CDROM:
+    case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
     case VIR_DOMAIN_DISK_DEVICE_LAST:
         break;
     }
@@ -1172,6 +1168,22 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver,
                                virDomainObjPtr vm,
                                virDomainDeviceDefPtr dev)
 {
+    virDomainDiskDefPtr disk = dev->data.disk;
+    virDomainDiskDefPtr orig_disk = NULL;
+
+    /* this API overloads media change semantics on disk hotplug
+     * for devices supporting media changes */
+    if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
+         disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
+        (orig_disk = virDomainDiskFindByBusAndDst(vm->def, disk->bus, disk->dst))) {
+        if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
+                                           disk->src, false) < 0)
+            return -1;
+
+        disk->src = NULL;
+        return 0;
+    }
+
     return qemuDomainAttachDeviceDiskLiveInternal(driver, vm, dev);
 }

-- 
2.17.1

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

On 9/27/18 11:09 AM, Peter Krempa wrote:
> Disk hotplug has slightly different semantics from media changing. Move
> the media change code out and add proper initialization of the new
> source object.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_driver.c  | 15 +----------
>  src/qemu/qemu_hotplug.c | 56 +++++++++++++++++++++++++----------------
>  2 files changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 64f0ad33e8..f609151152 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7851,12 +7851,6 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
>      virDomainDeviceDef oldDev = { .type = dev->type };
>      int ret = -1;
> 
> -    if (virDomainDiskTranslateSourcePool(disk) < 0)
> -        goto cleanup;
> -

Would removing this affect the subsequent virStorageSourceIsSameLocation
call?  Since def->disk->src is one of the "changed" values?


> -    if (qemuDomainDetermineDiskChain(driver, vm, disk, true) < 0)
> -        goto cleanup;
> -
>      if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
>                                                     disk->bus, disk->dst))) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -7885,16 +7879,9 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
>              goto cleanup;
>          }
> 
> -        /* Add the new disk src into shared disk hash table */
> -        if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0)
> -            goto cleanup;
> -
>          if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
> -                                           dev->data.disk->src, force) < 0) {
> -            ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk,
> -                                              vm->def->name));
> +                                           dev->data.disk->src, force) < 0)
>              goto cleanup;
> -        }
> 
>          dev->data.disk->src = NULL;
>      }
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 6227a130da..ef3f0cd8b3 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -737,6 +737,15 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> 
>      disk->src = newsrc;
> 
> +    if (virDomainDiskTranslateSourcePool(disk) < 0)
> +        goto cleanup;
> +
> +    if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0)
> +        goto cleanup;

Failures after this would need to call qemuRemoveSharedDisk

John

[...]

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