It can also be used for validation of input in qemuDomainSetBlockIoTune.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/qemu/qemu_validate.c | 100 ++++++++++++++++++++++++++---------------------
 src/qemu/qemu_validate.h |   4 ++
 2 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index eadf3af..6a27565 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2696,6 +2696,61 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
 }
 
 
+int
+qemuValidateDomainBlkdeviotune(const virDomainBlockIoTuneInfo *iotune,
+                               virQEMUCapsPtr qemuCaps)
+{
+    if (iotune->total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->total_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->read_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->write_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->total_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->read_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->write_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->total_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->read_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->write_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
+        iotune->size_iops_sec > QEMU_BLOCK_IOTUNE_MAX) {
+        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
+                       _("block I/O throttle limit must "
+                         "be no more than %llu using QEMU"),
+                       QEMU_BLOCK_IOTUNE_MAX);
+        return -1;
+    }
+
+    /* block I/O throttling 1.7 */
+    if (virDomainBlockIoTuneInfoHasMax(iotune) &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("there are some block I/O throttling parameters "
+                         "that are not supported with this QEMU binary"));
+        return -1;
+    }
+
+    /* block I/O group 2.4 */
+    if (iotune->group_name &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("the block I/O throttling group parameter is "
+                         "not supported with this QEMU binary"));
+        return -1;
+    }
+
+    /* block I/O throttling length 2.6 */
+    if (virDomainBlockIoTuneInfoHasMaxLength(iotune) &&
+        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("there are some block I/O throttling length parameters "
+                         "that are not supported with this QEMU binary"));
+        return -1;
+    }
+
+    return 0;
+}
+
+
 /**
  * qemuValidateDomainDeviceDefDiskBlkdeviotune:
  * @disk: disk configuration
@@ -2740,51 +2795,8 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const virDomainDiskDef *disk,
         }
     }
 
-    if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.total_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.read_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.write_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.total_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.read_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.write_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.total_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.read_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.write_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
-        disk->blkdeviotune.size_iops_sec > QEMU_BLOCK_IOTUNE_MAX) {
-        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
-                      _("block I/O throttle limit must "
-                        "be no more than %llu using QEMU"), QEMU_BLOCK_IOTUNE_MAX);
-        return -1;
-    }
-
-    /* block I/O throttling 1.7 */
-    if (virDomainBlockIoTuneInfoHasMax(&disk->blkdeviotune) &&
-        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("there are some block I/O throttling parameters "
-                         "that are not supported with this QEMU binary"));
+    if (qemuValidateDomainBlkdeviotune(&disk->blkdeviotune, qemuCaps) < 0)
         return -1;
-    }
-
-    /* block I/O group 2.4 */
-    if (disk->blkdeviotune.group_name &&
-        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("the block I/O throttling group parameter is "
-                         "not supported with this QEMU binary"));
-        return -1;
-    }
-
-    /* block I/O throttling length 2.6 */
-    if (virDomainBlockIoTuneInfoHasMaxLength(&disk->blkdeviotune) &&
-        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("there are some block I/O throttling length parameters "
-                         "that are not supported with this QEMU binary"));
-        return -1;
-    }
 
     return 0;
 }
diff --git a/src/qemu/qemu_validate.h b/src/qemu/qemu_validate.h
index b6c5441..21e7986 100644
--- a/src/qemu/qemu_validate.h
+++ b/src/qemu/qemu_validate.h
@@ -26,6 +26,10 @@
 #include "qemu_capabilities.h"
 #include "qemu_conf.h"
 
+int
+qemuValidateDomainBlkdeviotune(const virDomainBlockIoTuneInfo *iotune,
+                               virQEMUCapsPtr qemuCaps);
+
 int qemuValidateDomainDef(const virDomainDef *def,
                           void *opaque,
                           void *parseOpaque);
-- 
1.8.3.1

On Mon, Jan 11, 2021 at 12:49:55 +0300, Nikolay Shirokovskiy wrote:
> It can also be used for validation of input in qemuDomainSetBlockIoTune.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/qemu/qemu_validate.c | 100 ++++++++++++++++++++++++++---------------------
>  src/qemu/qemu_validate.h |   4 ++
>  2 files changed, 60 insertions(+), 44 deletions(-)
> 
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index eadf3af..6a27565 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -2696,6 +2696,61 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
>  }
>  
>  
> +int
> +qemuValidateDomainBlkdeviotune(const virDomainBlockIoTuneInfo *iotune,
> +                               virQEMUCapsPtr qemuCaps)
> +{

The check that group_name must be set along with other fields :

    /* group_name by itself is ignored by qemu */
    if (disk->blkdeviotune.group_name &&
        !virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune)) {
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                       _("group_name can be configured only together with "
                         "settings"));
        return -1;
    }


also belongs here.


> +    if (iotune->total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->total_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->read_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->write_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->total_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->read_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->write_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->total_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->read_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->write_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> +        iotune->size_iops_sec > QEMU_BLOCK_IOTUNE_MAX) {
> +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> +                       _("block I/O throttle limit must "
> +                         "be no more than %llu using QEMU"),
> +                       QEMU_BLOCK_IOTUNE_MAX);
> +        return -1;
> +    }

We also nowadays prefer if the error detail strings are not broken up,
but that's not a required change.

With the group name check moved too:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

ср, 3 мар. 2021 г. в 15:54, Peter Krempa <pkrempa@redhat.com>:

> On Mon, Jan 11, 2021 at 12:49:55 +0300, Nikolay Shirokovskiy wrote:
> > It can also be used for validation of input in qemuDomainSetBlockIoTune.
> >
> > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> > ---
> >  src/qemu/qemu_validate.c | 100
> ++++++++++++++++++++++++++---------------------
> >  src/qemu/qemu_validate.h |   4 ++
> >  2 files changed, 60 insertions(+), 44 deletions(-)
> >
> > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > index eadf3af..6a27565 100644
> > --- a/src/qemu/qemu_validate.c
> > +++ b/src/qemu/qemu_validate.c
> > @@ -2696,6 +2696,61 @@ qemuValidateDomainDeviceDefDiskFrontend(const
> virDomainDiskDef *disk,
> >  }
> >
> >
> > +int
> > +qemuValidateDomainBlkdeviotune(const virDomainBlockIoTuneInfo *iotune,
> > +                               virQEMUCapsPtr qemuCaps)
> > +{
>
> The check that group_name must be set along with other fields :
>
>     /* group_name by itself is ignored by qemu */
>     if (disk->blkdeviotune.group_name &&
>         !virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune)) {
>         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                        _("group_name can be configured only together with "
>                          "settings"));
>         return -1;
>     }
>
>
> also belongs here.
>

I cannot put this check into qemuValidateDomainBlkdeviotune. The thing is
I'm going to reuse this function
in qemuDomainSetBlockIoTune and in qemuDomainSetBlockIoTune is it fine to
have group_name only in input.
This means "move this disk to that io tune group" or "create io tune group
and put this disk into it" depending
on whether io tune with that name exists or not (this is what
qemuDomainSetBlockIoTuneDefaults do).


Nikolay



>
>
> > +    if (iotune->total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->total_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->read_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->write_iops_sec > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->total_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->read_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->write_bytes_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->total_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->read_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->write_iops_sec_max > QEMU_BLOCK_IOTUNE_MAX ||
> > +        iotune->size_iops_sec > QEMU_BLOCK_IOTUNE_MAX) {
> > +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
> > +                       _("block I/O throttle limit must "
> > +                         "be no more than %llu using QEMU"),
> > +                       QEMU_BLOCK_IOTUNE_MAX);
> > +        return -1;
> > +    }
>
> We also nowadays prefer if the error detail strings are not broken up,
> but that's not a required change.
>
> With the group name check moved too:
>
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
>
>