4:27 p.m.
On 1/18/19 1:59 PM, Daniel Henrique Barboza wrote:
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(a)in.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
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;
Prefer to see something like devinfo or diskinfo - what you look at
isn't a vm (virtual machine)!
+ 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)) {
This would seem to assuming something about the address type wouldn't
it? A _virDomainDeviceInfo is structure that uses it's @type in order
to determine what type of address is being used...
Look at tests/qemuxml2xmloutdata/disk-virtio.xml and wonder what would
happen...
Suggestion - look for virDomainDeviceInfoAddressIsEqual and maybe even
virDomainDefHasDeviceAddress
Perhaps even search through domain_conf.c for 'ndisks' and see how other
iterations are done.
In a way I'm wondering how we got this far where XML with a duplicated
address was accepted. Of course I haven't thought/looked that hard at
the hotplug path lately either.
John
+ /* 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;