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@xxxxxxxxxxxxx>
> ---
> 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 :|
