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