Re: [libvirt] [PATCHv2] qemu: Validate address type when attaching a disk device.
On Mon, Sep 07, 2015 at 07:22:01PM +0800, Ruifeng Bian wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Attach-device can hotplug a virtio disk device with any address type now,
it need to validate the address type before the attachment.
Coldplug a scsi disk device without checking the address type in current
version, this patch also fix the scsi disk problem.
---
src/qemu/qemu_driver.c | 8 ++++++++
src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 91eb661..af926fc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
if (virDomainDefAddImplicitControllers(vmdef) < 0)
return -1;
+ /* scsi disk should have an address type of driver */
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
+ (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("scsi disk cannot have an address of type
'%s'"),
+ virDomainDeviceAddressTypeToString(disk->info.type));
+ return -1;
+ }
This is the check for DISK_BUS_SCSI, qemuDomainAssignAddresses does the
check for DISK_BUS_VIRTIO.
Should we check other bus types as well?
I think the most important part of the bug is that we allow hotplugging
a device with broken configuration, then fail to unplug it because it is
broken. Here detach-device works with --config even with an incorrect
address type.
if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) <
0)
return -1;
break;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 63fafa6..4226650 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info,
priv->qemuCaps,
disk->dst))
goto cleanup;
+
+ /* virtio device should either have a ccw or pci address */
+ if (qemuDomainMachineIsS390CCW(vm->def) &&
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
+ if (!virDomainDeviceAddressIsValid(&disk->info,
+ VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("device cannot be attached without a valid CCW
address"));
+ goto cleanup;
+ }
Is hotplugging a device with a S390 type address impossible on a s390-ccw
machine?
+ } else {
+ if (!virDomainDeviceAddressIsValid(&disk->info,
+ VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("device cannot be attached without a valid PCI
address"));
These are not the only two options. At this point, the address type can
also be VIRTIO_S390 for non-CCW S390 machines, and ADDRESS_TYPE_NONE
(and a PCI address will be generated by virDomainPCIAddressEnsureAddr).
The checks seem to be copied from qemuDomainDetachVirtioDiskDevice. For
ancient QEMUs which did not support -device, we use the PCI address to
uniquely identify the device. In that case, detaching a virtio disk with
no address is not possible.
For all reasonable versions of QEMU, the device alias is used, so
detaching devices with other address types is possible.
But we explicitly require a PCI or CCW address, to make sure we will
unplug only the device requested by user. S390 seems to be missing here,
but that might be on purpose.
Allowing S390 addresses on hotplug without allowing them on hotunplug
won't fix the linked bug for S390 machines, but attaching a device with
no PCI address is a valid use case - libvirt will just generate one.
+ goto cleaup;
Please make sure 'make check' and 'make syntax-check' pass before
sending a patch to the list, as described on our hacking page:
http://libvirt.org/hacking.html
Jan
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)