[libvirt] [PATCH v1 1/1] qemu_hotplug.c: assert disk->info.alias unique before hotplug
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 generated alias against the aliases already presented in the disks in the VM definition. If a match happens, 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 [sudo] password for danielhb: error: Failed to attach device from /home/danielhb/hp-disk-dup.xml error: operation failed: attached disk conflicts with existing device id '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 | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a1c3ca999b..7c770211ab 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -875,6 +875,27 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } +/** + * qemuDomainDeviceAliasIsUnique: + * + * Searches the existing domain disks to check if a given alias is + * unique. */ +static bool +qemuDomainDeviceAliasIsUnique(virDomainDefPtr def, char *alias) +{ + int idx; + + for (idx = (def->ndisks - 1); idx >= 0; idx--) { + if (def->disks[idx]->info.alias && + (strcmp(def->disks[idx]->info.alias, alias) == 0)) { + + return false; + } + } + return true; +} + + /** * qemuDomainAttachDiskGeneric: * @@ -897,6 +918,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; + if (!qemuDomainDeviceAliasIsUnique(vm->def, disk->info.alias)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("attached disk conflicts with existing " + "device id '%s'"), disk->info.alias); + goto error; + } + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto error; -- 2.20.1
On Thu, Jan 17, 2019 at 17:23:07 -0200, 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 generated alias against the aliases already presented in the disks in the VM definition. If a match happens, 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 [sudo] password for danielhb: error: Failed to attach device from /home/danielhb/hp-disk-dup.xml error: operation failed: attached disk conflicts with existing device id 'scsi0-0-0-0'
The main problem here is that we don't check the duplicity of the <address> data not the alias itself. The alias is only the first symptom of this which might not occur in some configurations e.g. if the other disk with the same alias uses an useralias. I'd prefer if this is fixed properly by fixing/adding the address check.
On 1/18/19 5:13 AM, Peter Krempa wrote:
On Thu, Jan 17, 2019 at 17:23:07 -0200, 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 generated alias against the aliases already presented in the disks in the VM definition. If a match happens, 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 [sudo] password for danielhb: error: Failed to attach device from /home/danielhb/hp-disk-dup.xml error: operation failed: attached disk conflicts with existing device id 'scsi0-0-0-0' The main problem here is that we don't check the duplicity of the <address> data not the alias itself. The alias is only the first symptom of this which might not occur in some configurations e.g. if the other disk with the same alias uses an useralias.
I'd prefer if this is fixed properly by fixing/adding the address check.
Thanks for the review. I'll respin it with an address check instead of the alias check done here. DHB
participants (2)
-
Daniel Henrique Barboza -
Peter Krempa