On Mon, Nov 18, 2024 at 19:24:20 +0530, Harikumar R wrote:
From: Chun Feng Wu <danielwuwy@163.com>
Refactor iotune verification, and verify some rules
* Disk iotune validation can be reused for throttle group validation, refactor it into common method "virDomainDiskIoTuneValidate" * Add "virDomainDefValidateThrottleGroups" to validate throttle groups, which in turn calls "virDomainDiskIoTuneValidate" * Make sure referenced throttle group exists * Use "iotune" and "throttlefilters" exclusively for specific disk * Throttle filters cannot be used together with CDROM
Signed-off-by: Chun Feng Wu <danielwuwy@163.com> ---
Once again a bit too much going on in a single commit. The extraction of the code to +virDomainDiskIoTuneValidate would qualify for a individual commit. As said before; don't change it now as I'm forseeing more trouble if you do.
src/conf/domain_validate.c | 119 ++++++++++++++++++++++++++----------- src/qemu/qemu_driver.c | 6 ++ src/qemu/qemu_hotplug.c | 8 +++ 3 files changed, 99 insertions(+), 34 deletions(-)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index b352cd874a..916d5c9986 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -659,11 +659,55 @@ virDomainDiskDefValidateStartupPolicy(const virDomainDiskDef *disk) }
+static int +virDomainDiskIoTuneValidate(const virDomainBlockIoTuneInfo blkdeviotune) +{ + if ((blkdeviotune.total_bytes_sec && + blkdeviotune.read_bytes_sec) || + (blkdeviotune.total_bytes_sec && + blkdeviotune.write_bytes_sec)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write bytes_sec cannot be set at the same time")); + return -1; + } + + if ((blkdeviotune.total_iops_sec && + blkdeviotune.read_iops_sec) || + (blkdeviotune.total_iops_sec && + blkdeviotune.write_iops_sec)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write iops_sec cannot be set at the same time")); + return -1; + } + + if ((blkdeviotune.total_bytes_sec_max && + blkdeviotune.read_bytes_sec_max) || + (blkdeviotune.total_bytes_sec_max && + blkdeviotune.write_bytes_sec_max)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write bytes_sec_max cannot be set at the same time")); + return -1; + } + + if ((blkdeviotune.total_iops_sec_max && + blkdeviotune.read_iops_sec_max) || + (blkdeviotune.total_iops_sec_max && + blkdeviotune.write_iops_sec_max)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write iops_sec_max cannot be set at the same time")); + return -1; + } + + return 0; +} + + static int virDomainDiskDefValidate(const virDomainDef *def, const virDomainDiskDef *disk) { virStorageSource *next; + size_t i;
/* disk target is used widely in other code so it must be validated first */ if (!disk->dst) { @@ -713,41 +757,8 @@ virDomainDiskDefValidate(const virDomainDef *def, }
/* Validate IotuneParse */ - if ((disk->blkdeviotune.total_bytes_sec && - disk->blkdeviotune.read_bytes_sec) || - (disk->blkdeviotune.total_bytes_sec && - disk->blkdeviotune.write_bytes_sec)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write bytes_sec cannot be set at the same time")); + if (virDomainDiskIoTuneValidate(disk->blkdeviotune) < 0) return -1; - } - - if ((disk->blkdeviotune.total_iops_sec && - disk->blkdeviotune.read_iops_sec) || - (disk->blkdeviotune.total_iops_sec && - disk->blkdeviotune.write_iops_sec)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write iops_sec cannot be set at the same time")); - return -1; - } - - if ((disk->blkdeviotune.total_bytes_sec_max && - disk->blkdeviotune.read_bytes_sec_max) || - (disk->blkdeviotune.total_bytes_sec_max && - disk->blkdeviotune.write_bytes_sec_max)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write bytes_sec_max cannot be set at the same time")); - return -1; - } - - if ((disk->blkdeviotune.total_iops_sec_max && - disk->blkdeviotune.read_iops_sec_max) || - (disk->blkdeviotune.total_iops_sec_max && - disk->blkdeviotune.write_iops_sec_max)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write iops_sec_max cannot be set at the same time")); - return -1; - }
/* Reject disks with a bus type that is not compatible with the * given address type. The function considers only buses that are @@ -943,6 +954,26 @@ virDomainDiskDefValidate(const virDomainDef *def, return -1; }
+ /* referenced group should be defined */ + for (i = 0; i < disk->nthrottlefilters; i++) { + virDomainThrottleFilterDef *filter = disk->throttlefilters[i]; + if (!virDomainThrottleGroupByName(def, filter->group_name)) { + virReportError(VIR_ERR_XML_ERROR, + _("throttle group '%1$s' not found"), + filter->group_name); + return -1; + } + } + + if (disk->throttlefilters && + (disk->blkdeviotune.group_name || + virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("block 'throttlefilters' can't be used together with 'iotune' for disk '%1$s'"), + disk->dst); + return -1; + } + return 0; }
@@ -1843,6 +1874,23 @@ virDomainDefLaunchSecurityValidate(const virDomainDef *def)
#undef CHECK_BASE64_LEN
+static int +virDomainDefValidateThrottleGroups(const virDomainDef *def) +{ + size_t i; + + for (i = 0; i < def->nthrottlegroups; i++) { + virDomainThrottleGroupDef *throttleGroup = def->throttlegroups[i]; + + /* Validate Throttle Group */
Pointless comment
+ if (virDomainDiskIoTuneValidate(*throttleGroup) < 0) + return -1; + } + + return 0; +} + + static int virDomainDefValidateInternal(const virDomainDef *def, virDomainXMLOption *xmlopt) @@ -1901,6 +1949,9 @@ virDomainDefValidateInternal(const virDomainDef *def, if (virDomainDefLaunchSecurityValidate(def) < 0) return -1;
+ if (virDomainDefValidateThrottleGroups(def) < 0) + return -1; + return 0; }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d65d5fd2ff..2a5f58635c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14862,6 +14862,12 @@ qemuDomainDiskBlockIoTuneIsSupported(virDomainDiskDef *disk) return false; }
+ if (disk->throttlefilters) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("block 'iotune' can't be used together with 'throttlefilters' for disk '%1$s'"), disk->dst); + return false; + } + return true; }
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bf001aa902..8c41d27704 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -988,6 +988,14 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, bool releaseSeclabel = false; int ret = -1;
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
IIRC this is so that there's no trouble when ejecting the media; but I'd expect to see the same for cold-plug as well.
+ if (disk->nthrottlefilters > 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cdrom device with throttle filters isn't supported")); + return -1; + } + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("floppy device hotplug isn't supported")); -- 2.39.5 (Apple Git-154)