----- Original Message -----
From: "Michal Privoznik" <mprivozn@redhat.com> To: "Ruifeng Bian" <rbian@redhat.com>, libvir-list@redhat.com Sent: Wednesday, September 16, 2015 11:32:27 PM Subject: Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching a disk device.
On 09.09.2015 13:17, Ruifeng Bian wrote:
Bug fix for: 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.
Attaching a disk device with --config option need to check address type. Otherwise, the domain cloudn't be started normally. This patch add a basic check for address type.
Detaching a disk device with --config also need to check disk options, add qemuCheckDiskConfig() method in qemuDomainDetachDeviceConfig(). --- src/qemu/qemu_command.c | 44 +++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_driver.c | 2 ++ src/qemu/qemu_hotplug.c | 22 ++++++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ec5e3d4..a2c7483 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3579,12 +3579,54 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) goto error; } } + + if (disk->info.type) { + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + case VIR_DOMAIN_DISK_BUS_SCSI: + case VIR_DOMAIN_DISK_BUS_SATA: + case VIR_DOMAIN_DISK_BUS_FDC: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk cannot have an address of type '%s'"), + virDomainDeviceAddressTypeToString(disk->info.type)); + goto error; + } + break; + case VIR_DOMAIN_DISK_BUS_USB: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("usb disk cannot have an address of type '%s'"), + virDomainDeviceAddressTypeToString(disk->info.type)); + goto error; + } + break; + case VIR_DOMAIN_DISK_BUS_VIRTIO: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("virtio disk cannot have an address of type '%s'"), + virDomainDeviceAddressTypeToString(disk->info.type)); + goto error; + } + break; + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SD: + /* no address type currently, return false if address provided */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk cannot have an address of type '%s'"), + virDomainDeviceAddressTypeToString(disk->info.type)); + goto error; + } + } return 0; error: return -1; }
While this is technically correct I wonder if this should live here or in the config parsing part. I mean, these constraints are not qemu specific, are they?
I have thought about it. I'm not sure whether other hypervisor will use it, so I put it here. Okay, moving this part to domain_conf is better, I will do it. Thanks, Ruifeng
- /* Check whether the device address is using either 'ccw' or default s390 * address format and whether that's "legal" for the current qemu and/or * guest os.machine type. This is the corollary to the code which doesn't diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 91eb661..61424b0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8205,6 +8205,8 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; + if (qemuCheckDiskConfig(disk) < 0) + return -1; if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { virReportError(VIR_ERR_INVALID_ARG, _("no target device %s"), disk->dst); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 63fafa6..1f46647 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -335,6 +335,28 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps, disk->dst)) goto cleanup; + + 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; + } + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) { + if (!virDomainDeviceAddressIsValid(&disk->info, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("device cannot be attached without a valid S390 address")); + goto cleanup; + } + } 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")); + goto cleanup; + } }
for (i = 0; i < vm->def->ndisks; i++) {
Otherwise this is looking good.
Michal