[PATCH v2 1/1] qemu_hotplug.c: check disk address before hotplug

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

In a case where we want to hotplug the following disk:

<disk type='file' device='disk'>
    (...)
    <address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>

In a QEMU guest that has a single OS disk, as follows:

<disk type='file' device='disk'>
    (...)
    <address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>

What happens is that the existing guest disk will receive the ID
'scsi0-0-0-0' due to how Libvirt calculate the alias based on
the address in qemu_alias.c, qemuAssignDeviceDiskAlias. When hotplugging
a disk that happens to have the same address, Libvirt will calculate
the same ID to it and attempt to device_add. QEMU will refuse it:

$ virsh attach-device dhb hp-disk-dup.xml
error: Failed to attach device from hp-disk-dup.xml
error: internal error: unable to execute QEMU command 'device_add': Duplicate ID 'scsi0-0-0-0' for device

And Libvirt follows it up with a cleanup code in qemuDomainAttachDiskGeneric
that ends up removing what supposedly is a faulty hotplugged disk but, in
this case, ends up being the original guest disk. This happens because Libvirt
doesn't differentiate the error received by QMP device_add.

An argument can be made for how QMP device_add should provide a different
error code for this scenario. A quicker way to solve the problem is
presented in this patch: let us check the address of disk to be attached and
see if there is already a disk with the same address in the VM definition.
In this case, error out without calling device_add.

After this patch, this is the result of the previous attach-device call:

$ ./run tools/virsh attach-device dhb ~/hp-disk-dup.xml
error: Failed to attach device from /home/danielhb/hp-disk-dup.xml
error: operation failed: attached disk address conflicts with existing disk 'scsi0-0-0-0'

Reported-by: Srikanth Aithal <bssrikanth@xxxxxxxxxx>
Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>
---
 src/qemu/qemu_hotplug.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a1c3ca999b..4e6703f0b8 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -875,6 +875,36 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
 }
 
 
+/**
+ * qemuDomainFindDiskByAddress:
+ *
+ * Returns an existing disk in the VM definition that matches a given
+ * bus/controller/unit/target set, NULL in no match was found. */
+static virDomainDiskDefPtr
+qemuDomainFindDiskByAddress(virDomainDefPtr def,
+                            virDomainDeviceInfo info)
+{
+    virDomainDeviceInfo vm_info;
+    int idx;
+
+    for (idx = 0; idx < def->ndisks; idx++) {
+        vm_info = def->disks[idx]->info;
+        if ((vm_info.addr.drive.bus == info.addr.drive.bus) &&
+            (vm_info.addr.drive.controller == info.addr.drive.controller) &&
+            (vm_info.addr.drive.unit == info.addr.drive.unit)) {
+                /* Address does not have target to compare. We have
+                 * a match. */
+                if (!info.addr.drive.target)
+                    return def->disks[idx];
+                else if (vm_info.addr.drive.target &&
+                         vm_info.addr.drive.target == info.addr.drive.target)
+                    return def->disks[idx];
+        }
+    }
+    return NULL;
+}
+
+
 /**
  * qemuDomainAttachDiskGeneric:
  *
@@ -888,12 +918,21 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
     int ret = -1;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     qemuHotplugDiskSourceDataPtr diskdata = NULL;
+    virDomainDiskDefPtr vm_disk = NULL;
     char *devstr = NULL;
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
     if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0)
         goto cleanup;
 
+    if ((disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) &&
+        (vm_disk = qemuDomainFindDiskByAddress(vm->def, disk->info))) {
+        virReportError(VIR_ERR_OPERATION_FAILED,
+                       _("attached disk address conflicts with existing "
+                         "disk '%s'"), vm_disk->info.alias);
+        goto error;
+    }
+
     if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0)
         goto error;
 
-- 
2.20.1

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