[libvirt] [PATCH v2 0/1] qemu_hotplug.c: check address before hotplug
Changes in v2: - removed the function to check for alias - added a new function to check for address in the VM definition Daniel Henrique Barboza (1): qemu_hotplug.c: check disk address before hotplug src/qemu/qemu_hotplug.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) -- 2.20.1
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; + 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)) { + /* 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; -- 2.20.1
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;
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@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)!
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: + 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;
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@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)!
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. 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 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;
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@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)! 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;
participants (2)
-
Daniel Henrique Barboza -
John Ferlan