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@in.ibm.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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;