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>
* Update of code documentation comments.
Signed-off-by: Harikumar Rajkumar <harirajkumar230@gmail.com>
---
src/conf/domain_conf.c | 8 +++
src/conf/domain_validate.c | 118 ++++++++++++++++++++++++++-----------
src/qemu/qemu_driver.c | 13 ++++
src/qemu/qemu_hotplug.c | 8 +++
4 files changed, 113 insertions(+), 34 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 303aa7d1ae..0cc318d1f9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8008,6 +8008,14 @@ virDomainDiskDefThrottleFiltersParse(virDomainDiskDef *def,
if ((n = virXPathNodeSet("./throttlefilters/throttlefilter", ctxt, &nodes)) < 0)
return -1;
+ if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ if (n > 0) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cdrom device with throttle filters isn't supported"));
+ return -1;
+ }
+ }
+
if (n)
def->throttlefilters = g_new0(virDomainThrottleFilterDef *, n);
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 563558d920..d51682d155 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -685,11 +685,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) {
@@ -739,41 +783,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
@@ -974,6 +985,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;
}
@@ -1874,6 +1905,22 @@ 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];
+
+ if (virDomainDiskIoTuneValidate(*throttleGroup) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
virDomainDefValidateInternal(const virDomainDef *def,
virDomainXMLOption *xmlopt)
@@ -1932,6 +1979,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 afb41a4c89..6b1e8d01e0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6703,6 +6703,13 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef,
_("target %1$s already exists"), disk->dst);
return -1;
}
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ if (disk->nthrottlefilters > 0) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("cdrom device with throttle filters isn't supported"));
+ return -1;
+ }
+ }
if (virDomainDiskTranslateSourcePool(disk) < 0)
return -1;
if (qemuCheckDiskConfigAgainstDomain(vmdef, disk) < 0)
@@ -14861,6 +14868,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 75437164ca..ff29cd55f4 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -985,6 +985,14 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
bool releaseSeclabel = false;
int ret = -1;
+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+ 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)On Wed, Feb 19, 2025 at 22:27:16 +0530, Harikumar Rajkumar 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>
>
> * Update of code documentation comments.
>
> Signed-off-by: Harikumar Rajkumar <harirajkumar230@gmail.com>
> ---
> src/conf/domain_conf.c | 8 +++
> src/conf/domain_validate.c | 118 ++++++++++++++++++++++++++-----------
> src/qemu/qemu_driver.c | 13 ++++
> src/qemu/qemu_hotplug.c | 8 +++
> 4 files changed, 113 insertions(+), 34 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 303aa7d1ae..0cc318d1f9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8008,6 +8008,14 @@ virDomainDiskDefThrottleFiltersParse(virDomainDiskDef *def,
> if ((n = virXPathNodeSet("./throttlefilters/throttlefilter", ctxt, &nodes)) < 0)
> return -1;
>
> + if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> + if (n > 0) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("cdrom device with throttle filters isn't supported"));
> + return -1;
> + }
> + }
Normally adding any form of validation into the parser is not allowed
because it makes VMs vanish. as this is happening in hthe same series
it's technically allowable, but ...
> +
> if (n)
> def->throttlefilters = g_new0(virDomainThrottleFilterDef *, n);
>
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 563558d920..d51682d155 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -685,11 +685,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) {
> @@ -739,41 +783,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
> @@ -974,6 +985,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;
... really belongs here into the validation code.
> + }
> +
> return 0;
> }
>
> @@ -1874,6 +1905,22 @@ 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];
> +
> + if (virDomainDiskIoTuneValidate(*throttleGroup) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> static int
> virDomainDefValidateInternal(const virDomainDef *def,
> virDomainXMLOption *xmlopt)
> @@ -1932,6 +1979,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 afb41a4c89..6b1e8d01e0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6703,6 +6703,13 @@ qemuDomainAttachDeviceConfig(virDomainDef *vmdef,
> _("target %1$s already exists"), disk->dst);
> return -1;
> }
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> + if (disk->nthrottlefilters > 0) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("cdrom device with throttle filters isn't supported"));
> + return -1;
This is dead code, the validation refuses this.
> + }
> + }
> if (virDomainDiskTranslateSourcePool(disk) < 0)
> return -1;
> if (qemuCheckDiskConfigAgainstDomain(vmdef, disk) < 0)
> @@ -14861,6 +14868,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 75437164ca..ff29cd55f4 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -985,6 +985,14 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
> bool releaseSeclabel = false;
> int ret = -1;
>
> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> + if (disk->nthrottlefilters > 0) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("cdrom device with throttle filters isn't supported"));
> + return -1;
> + }
> + }
Dead code.
> +
> 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)
>
© 2016 - 2025 Red Hat, Inc.