[libvirt] [PATCH 3/7] qemu: check iotune params same for all disk in group

[Daniel P. Berrangé]

On Wed, Jan 08, 2020 at 09:49:27AM +0300, Nikolay Shirokovskiy wrote:
> Currently it is possible to start a domain which have disks
> in same iotune group and at the same time having different iotune
> params. Both params set are passed to qemu in command line and the one
> that is passed later down command line is get actually set.
> Let's prohibit such configurations.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>  src/conf/domain_conf.c   | 29 ++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |  4 ++++
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_command.c  | 41 +++++++++++++++++++++++++++++++++-------
>  src/qemu/qemu_command.h  |  3 +++
>  src/qemu/qemu_driver.c   |  2 +-
>  src/qemu/qemu_hotplug.c  |  2 +-
>  tests/qemublocktest.c    |  2 +-
>  8 files changed, 74 insertions(+), 10 deletions(-)
> 

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 29928ac1c8..94a49b7e4f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1156,6 +1156,7 @@ qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDefPtr disk)
>   */
>  static int
>  qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
> +                                const virDomainDef *def,
>                                  virQEMUCapsPtr qemuCaps)
>  {
>      /* group_name by itself is ignored by qemu */
> @@ -1167,6 +1168,27 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
>          return -1;
>      }
>  
> +    /* checking def here is only for calling from tests */
> +    if (disk->blkdeviotune.group_name && def) {

I'm not a fan of special casing for the test suite

> +        size_t i;
> +
> +        for (i = 0; i < def->ndisks; i++) {
> +            virDomainDiskDefPtr d = def->disks[i];
> +
> +            if (!d->blkdeviotune.group_name ||
> +                STRNEQ(disk->blkdeviotune.group_name, d->blkdeviotune.group_name))
> +                continue;
> +
> +            if (!virDomainBlockIoTuneInfoEqual(&disk->blkdeviotune,
> +                                              &d->blkdeviotune)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("different iotunes for disks %s and %s"),
> +                               disk->dst, d->dst);
> +                return -1;
> +            }
> +        }
> +    }
> +
>      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 ||

> diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
> index 3e9edb85f0..670e5bfab1 100644
> --- a/tests/qemublocktest.c
> +++ b/tests/qemublocktest.c
> @@ -204,7 +204,7 @@ testQemuDiskXMLToProps(const void *opaque)
>                                         VIR_DOMAIN_DEF_PARSE_STATUS)))
>          goto cleanup;
>  
> -    if (qemuCheckDiskConfig(disk, data->qemuCaps) < 0 ||
> +    if (qemuCheckDiskConfig(disk, NULL, data->qemuCaps) < 0 ||

Passing invalid parameters to an API, only in the test suite, has tended
to cause pain for us in the long term.

Can we create a valid virDomainDef to pass in - it doens't have to
contain a full VM really - just enough functionality to satisify the
method

>          qemuDomainDeviceDefValidateDisk(disk, data->qemuCaps) < 0) {
>          VIR_TEST_VERBOSE("invalid configuration for disk");
>          goto cleanup;

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|