From 33188c9fcb7babe80ecac6267b17a32c87bb7e60 Mon Sep 17 00:00:00 2001 From: John Ferlan Date: Tue, 29 Jul 2014 09:21:05 -0400 Subject: [PATCH] Perform disk config validity checking for attach-device config https://bugzilla.redhat.com/show_bug.cgi?id=1078126 Using 'virsh attach-device --config' (or --persistent) to attach a file backed lun device will succeed; however, subsequent domain restarts will result in failure because the configuration of a file backed lun is not supported. Although allowing 'illegal configurations' is something that can be allowed, it may not be practical in this case. Generally, when attaching a device to a domain means the domain must be running. A way around this is using the --config (or --persistent) option. When an attach is done to a running domain, a temporary configuration is modified first followed by the live update. The live update will make a number of disk validity checks when building the qemu command to attach the disk. If any fail, then change is rejected. Rather than allow a potentially illegal combination, adjust the code in the configuration path to make the same checks as the running path will make with respect to disk validity checks. This way we avoid having the potential for some subsequent start/reboot to fail because an illegal combination was allowed. NB: The live path still checks the configuration since it is possible to just do --live guest modification... --- src/qemu/qemu_command.c | 127 +++++++++++++++++++++++----------------- src/qemu/qemu_command.h | 2 + src/qemu/qemu_driver.c | 2 + 3 files changed, 76 insertions(+), 55 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 903dafdb79..0d7b12dade 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3216,6 +3216,73 @@ qemuGetDriveSourceString(virStorageSourcePtr src, } +/* Perform disk definition config validity checks */ +int +qemuCheckDiskConfig(virDomainDiskDefPtr disk) +{ + if (virDiskNameToIndex(disk->dst) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported disk type '%s'"), disk->dst); + goto error; + } + + if (disk->wwn) { + if ((disk->bus != VIR_DOMAIN_DISK_BUS_IDE) && + (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only ide and scsi disk support wwn")); + goto error; + } + } + + if ((disk->vendor || disk->product) && + disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only scsi disk supports vendor and product")); + goto error; + } + + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { + /* make sure that both the bus supports type='lun' (SG_IO). */ + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO && + disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk device='lun' is not supported for bus='%s'"), + virDomainDiskQEMUBusTypeToString(disk->bus)); + goto error; + } + if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { + if (disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk device='lun' is not supported " + "for protocol='%s'"), + virStorageNetProtocolTypeToString(disk->src->protocol)); + goto error; + } + } else if (!virDomainDiskSourceIsBlockType(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk device='lun' is only valid for block " + "type disk source")); + goto error; + } + if (disk->wwn) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting wwn is not supported for lun device")); + goto error; + } + if (disk->vendor || disk->product) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Setting vendor or product is not supported " + "for lun device")); + goto error; + } + } + return 0; + error: + return -1; +} + + char * qemuBuildDriveStr(virConnectPtr conn, virDomainDiskDefPtr disk, @@ -3602,68 +3669,18 @@ qemuBuildDriveDevStr(virDomainDefPtr def, { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); - int idx = virDiskNameToIndex(disk->dst); int controllerModel; - if (idx < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported disk type '%s'"), disk->dst); + if (qemuCheckDiskConfig(disk) < 0) goto error; - } - - if (disk->wwn) { - if ((disk->bus != VIR_DOMAIN_DISK_BUS_IDE) && - (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only ide and scsi disk support wwn")); - goto error; - } - } - - if ((disk->vendor || disk->product) && - disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only scsi disk supports vendor and product")); - goto error; - } + /* Live only checks */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { - /* make sure that both the bus and the qemu binary support - * type='lun' (SG_IO). - */ - if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO && - disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk device='lun' is not supported for bus='%s'"), - bus); - goto error; - } - if (disk->src->type == VIR_STORAGE_TYPE_NETWORK) { - if (disk->src->protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk device='lun' is not supported for protocol='%s'"), - virStorageNetProtocolTypeToString(disk->src->protocol)); - goto error; - } - } else if (!virDomainDiskSourceIsBlockType(disk->src)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk device='lun' is only valid for block type disk source")); - goto error; - } + /* make sure that the qemu binary supports type='lun' (SG_IO). */ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk device='lun' is not supported by this QEMU")); - goto error; - } - if (disk->wwn) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Setting wwn is not supported for lun device")); - goto error; - } - if (disk->vendor || disk->product) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Setting vendor or product is not supported " - "for lun device")); + _("disk device='lun' is not supported by " + "this QEMU")); goto error; } } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 95f3fc9526..633ff71bd2 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -271,4 +271,6 @@ qemuParseKeywords(const char *str, int qemuGetDriveSourceString(virStorageSourcePtr src, virConnectPtr conn, char **source); + +int qemuCheckDiskConfig(virDomainDiskDefPtr disk); #endif /* __QEMU_COMMAND_H__*/ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02088ccd45..fa0ad5ac9d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6737,6 +6737,8 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, _("target %s already exists"), disk->dst); return -1; } + if (qemuCheckDiskConfig(disk) < 0) + return -1; if (virDomainDiskInsert(vmdef, disk)) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ -- GitLab