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

[John Ferlan]

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