[libvirt] [PATCH v2 1/1] qemu_hotplug.c: check disk address before hotplug
Daniel Henrique Barboza
danielhb413 at gmail.com
Sat Jan 26 14:18:32 UTC 2019
On 1/26/19 10:31 AM, John Ferlan wrote:
>
> On 1/26/19 6:37 AM, Daniel Henrique Barboza wrote:
>>
>> On 1/25/19 8:27 PM, John Ferlan wrote:
>>> 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)!
>> Ok!
>>
>>>> + 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...
>> Yeah, this function is being called under a conditional inside
>> qemuDomainAttachDiskGeneric:
>>
> qemuDomainAttachDiskGeneric has 3 callers:
>
> qemuDomainAttachVirtioDiskDevice
> qemuDomainAttachSCSIDisk
> qemuDomainAttachUSBMassStorageDevice
>
> So why wouldn't the same possibility exist for other address types? A
> very generically named function qemuDomainFindDiskByAddress is doing
> something that's SCSI specific.
>
> Still prior to that those 3 functions above are called by
>
> qemuDomainAttachDeviceDiskLiveInternal
>
> Within that context there's a call to virDomainDiskDefCheckDuplicateInfo
> which perhaps may be place to check that if the "to be added" disk has
> an address e.g. call virDomainDeviceInfoAddressIsEqual and if it returns
> true, then you have an error.
That's a good point. Perhaps there is a way to do this verification
regardless of address type.
>
> Still this is something that virDomainDefValidate processing does via
> virDomainDefCheckDuplicateDiskInfo and I'm wondering why
> qemuDomainAttachDeviceLiveAndConfig doesn't do during it's processing of
> virDomainDeviceDefParse, but I don't have the cycles right now to
> investigate
Fair enough. I'll investigate this further before sending a v3. Perhaps
an RFC
patch to explore the idea might be a good idea too.
DHB
>
>
> John
>
>> + if ((disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) &&
>> + (vm_disk = qemuDomainFindDiskByAddress(vm->def, disk->info))) {
>>
>> It would be clearer to simply move this conditional inside the function
>> I guess.
>>
>>
>>
>>> 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.
>> I'll take a look into those.
>>
>> Just to make my intentions clear: in v1 I just made an alias check
>> after qemuAssingDeviceDiskAlias to see if the calculated alias already
>> exists
>> in the domain def. With this new patch the idea was to check for the same
>> address instead - giving that the alias code uses the address to
>> calculate, the
>> effect would be the same as v1 with a plus of avoiding a collision with
>> user
>> created aliases. The logic of address checking was inspired by how the
>> address
>> is used inside qemuAssignDeviceDiskAlias.
>>
>>
>>
>> Thanks,
>>
>>
>> DHB
>>
>>
>>
>>
>>> 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;
>>>>
More information about the libvir-list
mailing list