Re: [PATCH 03/23] qemu: reuse validation in qemuDomainSetBlockIoTune

[Peter Krempa]

On Mon, Jan 11, 2021 at 12:49:56 +0300, Nikolay Shirokovskiy wrote:
> There is a little difference though in removed and reused code in
> qemuDomainSetBlockIoTune.
> 
> First, removed code checked 'set_fields' instead of tune itself. set_fields is
> true whenever corresponding virDomainBlockIoTuneInfoHas* it true. But
> additionnaly it is true when 0 values are passed explicitly. So removed code
> also failed if reset value is passed and qemu does not support the resetted
> field. I guess this is not very useful check and can be dropped as result of
> this patch. If field is not supported then it is reported as 0 and resetting 
> it
> to 0 is just NOP.

I'd say that it's an acceptable change.

> Second, check for QEMU_BLOCK_IOTUNE_MAX is added but it is alredy checked
> above and I'm going to remove the above check.

I'd not mention this.

> 
> Third, now check of qemuDomainSetBlockIoTune is done also if changing only
> persistent config is requested. It is good because otherwise we will create
> invalid config which can not be created thru define API due to existing qemu
> validation code.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/qemu/qemu_driver.c | 31 ---
>  1 file changed, 4 insertions(+), 27 deletions(-)

Reviewed-by: Peter Krempa 

[PATCH 03/23] qemu: reuse validation in qemuDomainSetBlockIoTune

[Nikolay Shirokovskiy]

There is a little difference though in removed and reused code in
qemuDomainSetBlockIoTune.

First, removed code checked 'set_fields' instead of tune itself. set_fields is
true whenever corresponding virDomainBlockIoTuneInfoHas* it true. But
additionnaly it is true when 0 values are passed explicitly. So removed code
also failed if reset value is passed and qemu does not support the resetted
field. I guess this is not very useful check and can be dropped as result of
this patch. If field is not supported then it is reported as 0 and resetting it
to 0 is just NOP.

Second, check for QEMU_BLOCK_IOTUNE_MAX is added but it is alredy checked
above and I'm going to remove the above check.

Third, now check of qemuDomainSetBlockIoTune is done also if changing only
persistent config is requested. It is good because otherwise we will create
invalid config which can not be created thru define API due to existing qemu
validation code.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_driver.c | 31 ---
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b7f22c2..7337464 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -53,6 +53,7 @@
 #include "qemu_namespace.h"
 #include "qemu_saveimage.h"
 #include "qemu_snapshot.h"
+#include "qemu_validate.h"
 
 #include "virerror.h"
 #include "virlog.h"
@@ -16210,6 +16211,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 
 virDomainBlockIoTuneInfoCopy(, _info);
 
+if (qemuValidateDomainBlkdeviotune(, priv->qemuCaps) < 0)
+goto endjob;
+
 if (def) {
 supportMaxOptions = virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_DRIVE_IOTUNE_MAX);
@@ -16218,33 +16222,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
 supportMaxLengthOptions =
 virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH);
 
-if (!supportMaxOptions &&
-(set_fields & (QEMU_BLOCK_IOTUNE_SET_BYTES_MAX |
-   QEMU_BLOCK_IOTUNE_SET_IOPS_MAX |
-   QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS))) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("a block I/O throttling parameter is not "
- "supported with this QEMU binary"));
- goto endjob;
-}
-
-if (!supportGroupNameOption &&
-(set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("the block I/O throttling group parameter is not "
- "supported with this QEMU binary"));
- goto endjob;
-}
-
-if (!supportMaxLengthOptions &&
-(set_fields & (QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH |
-   QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH))) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("a block I/O throttling length parameter is not "
- "supported with this QEMU binary"));
- goto endjob;
-}
-
 if (!(disk = qemuDomainDiskByName(def, path)))
 goto endjob;
 
-- 
1.8.3.1