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@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;
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;
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
