10:40 p.m.
----- Original Message -----
From: "John Ferlan" <jferlan(a)redhat.com>
To: "Ruifeng Bian" <rbian(a)redhat.com>, libvir-list(a)redhat.com
Sent: Wednesday, September 30, 2015 5:31:32 AM
Subject: Re: [libvirt] [PATCH v4] qemu: Validate address type when attaching a disk
device.
On 09/17/2015 03:35 AM, 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().
>
> Add virDomainDeviceAddressTypeIsValid method in domain_conf to check disk
> address type. Address type should match with disk driver, report error
> messages if any mismatch.
>
> Signed-off-by: Ruifeng Bian <rbian(a)redhat.com>
> ---
> src/conf/domain_conf.c | 32 ++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_command.c | 7 +++++++
> src/qemu/qemu_driver.c | 2 ++
> src/qemu/qemu_hotplug.c | 22 ++++++++++++++++++++++
> 6 files changed, 65 insertions(+)
>
Just so this isn't left "hanging"... Michal has generated an alternate
method to address the issue, see:
http://www.redhat.com/archives/libvir-list/2015-September/msg00909.html
I think there could be "synergies" between the two and left some
thoughts in the other patch which use some of this code.
Some other comments below since they may be useful...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6df1618..f86760b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3200,6 +3200,38 @@ int
> virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
> return 0;
> }
>
> +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
should be:
bool
virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
> +{
> + 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)
> + return 1;
return false;
> + break;
> + case VIR_DOMAIN_DISK_BUS_USB:
> + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB)
> + return 1;
return false;
> + 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)
> + return 1;
return false;
> + 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
> */
> + return 1;
return false;
> + }
> + }
> + return 0;
return true;
> +}
> +
> virDomainDeviceInfoPtr
> virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
> {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f043554..337fe51 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2543,6 +2543,7 @@ virDomainDeviceDefPtr
> virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
> virDomainXMLOptionPtr
> xmlopt);
> int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info,
> int type);
> +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk);
s/int/bool, probably unnecessary though
> virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr
> device);
> int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
> virDomainDeviceInfoPtr src);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5b3da83..6cd5b9e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -229,6 +229,7 @@ virDomainDefPostParse;
> virDomainDefSetMemoryInitial;
> virDomainDeleteConfig;
> virDomainDeviceAddressIsValid;
> +virDomainDeviceAddressTypeIsValid;
probably unnecessary
> virDomainDeviceAddressTypeToString;
> virDomainDeviceDefCheckUnsupportedMemoryDevice;
> virDomainDeviceDefCopy;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 25f57f2..56ba08d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3606,6 +3606,13 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
> goto error;
> }
> }
> +
> + if (virDomainDeviceAddressTypeIsValid(disk)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk cannot have an address of type
'%s'"),
> +
> virDomainDeviceAddressTypeToString(disk->info.type));
how about disk target '%s' using bus '%s' cannot have an address of type
'%s', where target would be disk->dst and the bus would be decoded via
virDomainDiskBusTypeToString(disk->bus), results in error message such as:
error: unsupported configuration: disk target 'vde' using bus 'virtio'
cannot have an address of type 'drive'
Thanks, the message is clear now.
> + goto error;
> + }
But I'm not convinced the check should be put it here.
> return 0;
> error:
> return -1;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index fcf86b6..26c1502 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8208,6 +8208,8 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> switch ((virDomainDeviceType) dev->type) {
> case VIR_DOMAIN_DEVICE_DISK:
> disk = dev->data.disk;
> + if (qemuCheckDiskConfig(disk) < 0)
> + return -1;
Why are we checking at Detach? Doesn't that already fail? Or is this
just to get the "new" error message?
If we just want to detach a disk without attaching it before, it's necessary to
check the configuration here.
> 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 e84a78d..6156243 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -336,6 +336,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;
> + }
The whole purpose of qemuCheckCCWS390AddressSupport is to avoid adding
more places where these type lines need to be added.
Yes, thanks.
John
> + } 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++) {
>