----- Original Message -----
From: "Michal Privoznik" <mprivozn(a)redhat.com>
To: "Ruifeng Bian" <rbian(a)redhat.com>, libvir-list(a)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