On Mon, Jan 11, 2021 at 12:49:55 +0300, Nikolay Shirokovskiy wrote: 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. 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(a)redhat.com&gt;

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 <nshirokovskiy(a)virtuozzo.com&gt; --- 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, &conf_info); + if (qemuValidateDomainBlkdeviotune(&info, 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

This is checked in below call to qemuValidateDomainBlkdeviotune now. Note that qemuValidateDomainBlkdeviotune does not check *_max_length values as we do here. But I guess this is for good. I tried setting high _max_lengh values and looks like their limits depend on *_max values and much less then 1000000000000000LL. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7337464..984b45d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16121,13 +16121,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, for (i = 0; i < nparams; i++) { virTypedParameterPtr param = &params[i]; - if (param->value.ul > QEMU_BLOCK_IOTUNE_MAX) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, - _("block I/O throttle limit value must" - " be no more than %llu"), QEMU_BLOCK_IOTUNE_MAX); - goto endjob; - } - SET_IOTUNE_FIELD(total_bytes_sec, BYTES, TOTAL_BYTES_SEC); SET_IOTUNE_FIELD(read_bytes_sec, BYTES, READ_BYTES_SEC); SET_IOTUNE_FIELD(write_bytes_sec, BYTES, WRITE_BYTES_SEC); -- 1.8.3.1

virDomainBlockIoTuneValidate can be reused in virDomainSetBlockIoTune implementations. And also simplify if conditions. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 78 +++++++++++++++++++++++++----------------------- src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + 3 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 349fc28..173424a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8627,6 +8627,45 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, return 0; } + +int +virDomainBlockIoTuneValidate(virDomainBlockIoTuneInfoPtr iotune) +{ + if (iotune->total_bytes_sec && + (iotune->read_bytes_sec || iotune->write_bytes_sec)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write bytes_sec " + "cannot be set at the same time")); + return -1; + } + + if (iotune->total_iops_sec && + (iotune->read_iops_sec || iotune->write_iops_sec)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write iops_sec " + "cannot be set at the same time")); + return -1; + } + + if (iotune->total_bytes_sec_max && + (iotune->read_bytes_sec_max || iotune->write_bytes_sec_max)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write bytes_sec_max " + "cannot be set at the same time")); + return -1; + } + + if (iotune->total_iops_sec_max && + (iotune->read_iops_sec_max || iotune->write_iops_sec_max)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("total and read/write iops_sec_max " + "cannot be set at the same time")); + return -1; + } + + return 0; +} + #define PARSE_IOTUNE(val) \ if (virXPathULongLong("string(./iotune/" #val ")", \ ctxt, &def->blkdeviotune.val) == -2) { \ @@ -8665,45 +8704,8 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, def->blkdeviotune.group_name = virXPathString("string(./iotune/group_name)", ctxt); - if ((def->blkdeviotune.total_bytes_sec && - def->blkdeviotune.read_bytes_sec) || - (def->blkdeviotune.total_bytes_sec && - def->blkdeviotune.write_bytes_sec)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write bytes_sec " - "cannot be set at the same time")); - return -1; - } - - if ((def->blkdeviotune.total_iops_sec && - def->blkdeviotune.read_iops_sec) || - (def->blkdeviotune.total_iops_sec && - def->blkdeviotune.write_iops_sec)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write iops_sec " - "cannot be set at the same time")); - return -1; - } - - if ((def->blkdeviotune.total_bytes_sec_max && - def->blkdeviotune.read_bytes_sec_max) || - (def->blkdeviotune.total_bytes_sec_max && - def->blkdeviotune.write_bytes_sec_max)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write bytes_sec_max " - "cannot be set at the same time")); + if (virDomainBlockIoTuneValidate(&def->blkdeviotune) < 0) return -1; - } - - if ((def->blkdeviotune.total_iops_sec_max && - def->blkdeviotune.read_iops_sec_max) || - (def->blkdeviotune.total_iops_sec_max && - def->blkdeviotune.write_iops_sec_max)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("total and read/write iops_sec_max " - "cannot be set at the same time")); - return -1; - } return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ec43bbe..3c42313 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3963,3 +3963,6 @@ virHostdevIsMdevDevice(const virDomainHostdevDef *hostdev) bool virHostdevIsVFIODevice(const virDomainHostdevDef *hostdev) ATTRIBUTE_NONNULL(1); + +int +virDomainBlockIoTuneValidate(virDomainBlockIoTuneInfoPtr iotune); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c325040..bfe3ee7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -236,6 +236,7 @@ virDomainBlockIoTuneInfoHasAny; virDomainBlockIoTuneInfoHasBasic; virDomainBlockIoTuneInfoHasMax; virDomainBlockIoTuneInfoHasMaxLength; +virDomainBlockIoTuneValidate; virDomainBootTypeFromString; virDomainBootTypeToString; virDomainCapabilitiesPolicyTypeToString; -- 1.8.3.1

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 984b45d..117c7b7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16170,37 +16170,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, #undef SET_IOTUNE_FIELD - if ((info.total_bytes_sec && info.read_bytes_sec) || - (info.total_bytes_sec && info.write_bytes_sec)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of bytes_sec " - "cannot be set at the same time")); - goto endjob; - } - - if ((info.total_iops_sec && info.read_iops_sec) || - (info.total_iops_sec && info.write_iops_sec)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of iops_sec " - "cannot be set at the same time")); - goto endjob; - } - - if ((info.total_bytes_sec_max && info.read_bytes_sec_max) || - (info.total_bytes_sec_max && info.write_bytes_sec_max)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of bytes_sec_max " - "cannot be set at the same time")); + if (virDomainBlockIoTuneValidate(&info) < 0) goto endjob; - } - - if ((info.total_iops_sec_max && info.read_iops_sec_max) || - (info.total_iops_sec_max && info.write_iops_sec_max)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of iops_sec_max " - "cannot be set at the same time")); - goto endjob; - } virDomainBlockIoTuneInfoCopy(&info, &conf_info); -- 1.8.3.1

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/test/test_driver.c | 32 +------------------------------- 1 file changed, 1 insertion(+), 31 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 29c4c86..4eb04d6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3780,38 +3780,8 @@ testDomainSetBlockIoTune(virDomainPtr dom, } #undef SET_IOTUNE_FIELD - if ((info.total_bytes_sec && info.read_bytes_sec) || - (info.total_bytes_sec && info.write_bytes_sec)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of bytes_sec " - "cannot be set at the same time")); - goto cleanup; - } - - if ((info.total_iops_sec && info.read_iops_sec) || - (info.total_iops_sec && info.write_iops_sec)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of iops_sec " - "cannot be set at the same time")); + if (virDomainBlockIoTuneValidate(&info) < 0) goto cleanup; - } - - if ((info.total_bytes_sec_max && info.read_bytes_sec_max) || - (info.total_bytes_sec_max && info.write_bytes_sec_max)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of bytes_sec_max " - "cannot be set at the same time")); - goto cleanup; - } - - if ((info.total_iops_sec_max && info.read_iops_sec_max) || - (info.total_iops_sec_max && info.write_iops_sec_max)) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("total and read/write of iops_sec_max " - "cannot be set at the same time")); - goto cleanup; - } - #define TEST_BLOCK_IOTUNE_MAX_CHECK(FIELD, FIELD_MAX) \ do { \ -- 1.8.3.1

Currenly API is not very convinient when switching from read/write to total tunes back and forth. read/write and total tunes can not be set simulaneously so one need to choose one. Now if for example total_bytes_sec and total_bytes_sec_max are set and we set read_bytes_sec only then API fails. The issue is new max settings are copied from the old ones in this case and as a result after defaults are applied we got settings for read_bytes_sec and total_bytes_sec_max which is incorrect. In order to handle such situation nicely let's reset max settings if appropriate avg settings is reseted explicitly or implicitly. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 117c7b7..9093baf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15901,6 +15901,22 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, SET_IOTUNE_DEFAULTS(IOPS_MAX, iops_sec_max); #undef SET_IOTUNE_DEFAULTS +#define RESET_IOTUNE_MAX(BOOL, FIELD) \ + do { \ + if (set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL) { \ + if (!newinfo->total_##FIELD) \ + newinfo->total_##FIELD##_max = 0; \ + if (!newinfo->read_##FIELD) \ + newinfo->read_##FIELD##_max = 0; \ + if (!newinfo->write_##FIELD) \ + newinfo->write_##FIELD##_max = 0; \ + } \ + } while (0) + + RESET_IOTUNE_MAX(BYTES, bytes_sec); + RESET_IOTUNE_MAX(IOPS, iops_sec); +#undef RESET_IOTUNE_MAX + if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS)) newinfo->size_iops_sec = oldinfo->size_iops_sec; if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME)) @@ -16210,17 +16226,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, do { \ if (info.val##_max) { \ if (!info.val) { \ - if (QEMU_BLOCK_IOTUNE_SET_##_bool) { \ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ - _("cannot reset '%s' when " \ - "'%s' is set"), \ - #val, #val "_max"); \ - } else { \ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ - _("value '%s' cannot be set if " \ - "'%s' is not set"), \ - #val "_max", #val); \ - } \ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ + _("value '%s' cannot be set if " \ + "'%s' is not set"), \ + #val "_max", #val); \ goto endjob; \ } \ if (info.val##_max < info.val) { \ -- 1.8.3.1

Now only qemu and test drivers support iotunes and for both of them this check makes sense. I guess there is little chance that this patch will break loading of some domains with incorrect config though. If this is the issue then we can put this common check to a different place. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 173424a..800bca5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8663,6 +8663,35 @@ virDomainBlockIoTuneValidate(virDomainBlockIoTuneInfoPtr iotune) return -1; } +#define CHECK_MAX(val) \ + do { \ + if (iotune->val##_max) { \ + if (!iotune->val) { \ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ + _("value '%s' cannot be set if " \ + "'%s' is not set"), \ + #val "_max", #val); \ + return -1; \ + } \ + if (iotune->val##_max < iotune->val) { \ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ + _("value '%s' cannot be " \ + "smaller than '%s'"), \ + #val "_max", #val); \ + return -1; \ + } \ + } \ + } while (false) + + CHECK_MAX(total_bytes_sec); + CHECK_MAX(read_bytes_sec); + CHECK_MAX(write_bytes_sec); + CHECK_MAX(total_iops_sec); + CHECK_MAX(read_iops_sec); + CHECK_MAX(write_iops_sec); + +#undef CHECK_MAX + return 0; } -- 1.8.3.1

These checks are now in the above call to virDomainBlockIoTuneValidate. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9093baf..02b28f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16222,35 +16222,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0) goto endjob; -#define CHECK_MAX(val, _bool) \ - do { \ - if (info.val##_max) { \ - if (!info.val) { \ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ - _("value '%s' cannot be set if " \ - "'%s' is not set"), \ - #val "_max", #val); \ - goto endjob; \ - } \ - if (info.val##_max < info.val) { \ - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ - _("value '%s' cannot be " \ - "smaller than '%s'"), \ - #val "_max", #val); \ - goto endjob; \ - } \ - } \ - } while (false) - - CHECK_MAX(total_bytes_sec, BYTES); - CHECK_MAX(read_bytes_sec, BYTES); - CHECK_MAX(write_bytes_sec, BYTES); - CHECK_MAX(total_iops_sec, IOPS); - CHECK_MAX(read_iops_sec, IOPS); - CHECK_MAX(write_iops_sec, IOPS); - -#undef CHECK_MAX - /* blockdev-based qemu doesn't want to set the throttling when a cdrom * is empty. Skip the monitor call here since we will set the throttling * once new media is inserted */ -- 1.8.3.1

These checks are now in the above call to virDomainBlockIoTuneValidate. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/test/test_driver.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4eb04d6..299be2a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3783,25 +3783,6 @@ testDomainSetBlockIoTune(virDomainPtr dom, if (virDomainBlockIoTuneValidate(&info) < 0) goto cleanup; -#define TEST_BLOCK_IOTUNE_MAX_CHECK(FIELD, FIELD_MAX) \ - do { \ - if (info.FIELD > info.FIELD_MAX) { \ - virReportError(VIR_ERR_INVALID_ARG, \ - _("%s cannot be set higher than %s "), \ - #FIELD, #FIELD_MAX); \ - goto cleanup; \ - } \ - } while (0); - - TEST_BLOCK_IOTUNE_MAX_CHECK(total_bytes_sec, total_bytes_sec_max); - TEST_BLOCK_IOTUNE_MAX_CHECK(read_bytes_sec, read_bytes_sec_max); - TEST_BLOCK_IOTUNE_MAX_CHECK(write_bytes_sec, write_bytes_sec_max); - TEST_BLOCK_IOTUNE_MAX_CHECK(total_iops_sec, total_iops_sec_max); - TEST_BLOCK_IOTUNE_MAX_CHECK(read_iops_sec, read_iops_sec_max); - TEST_BLOCK_IOTUNE_MAX_CHECK(write_iops_sec, write_iops_sec_max); - -#undef TEST_BLOCK_IOTUNE_MAX_CHECK - virDomainDiskSetBlockIOTune(conf_disk, &info); info.group_name = NULL; -- 1.8.3.1

We need extra variable in macros thus let's add do {} while (0) wrapping. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02b28f0..15ccf90 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15889,11 +15889,13 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, qemuBlockIoTuneSetFlags set_fields) { #define SET_IOTUNE_DEFAULTS(BOOL, FIELD) \ - if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL)) { \ - newinfo->total_##FIELD = oldinfo->total_##FIELD; \ - newinfo->read_##FIELD = oldinfo->read_##FIELD; \ - newinfo->write_##FIELD = oldinfo->write_##FIELD; \ - } + do { \ + if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL)) { \ + newinfo->total_##FIELD = oldinfo->total_##FIELD; \ + newinfo->read_##FIELD = oldinfo->read_##FIELD; \ + newinfo->write_##FIELD = oldinfo->write_##FIELD; \ + } \ + } while (0) SET_IOTUNE_DEFAULTS(BYTES, bytes_sec); SET_IOTUNE_DEFAULTS(BYTES_MAX, bytes_sec_max); @@ -15935,13 +15937,15 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, * our newinfo is clearing, then set max_length based on whether we * have a value in the family set/defined. */ #define SET_MAX_LENGTH(BOOL, FIELD) \ - if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL)) \ - newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length; \ - else if ((set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL) && \ - oldinfo->FIELD##_max_length && \ - !newinfo->FIELD##_max_length) \ - newinfo->FIELD##_max_length = (newinfo->FIELD || \ - newinfo->FIELD##_max) ? 1 : 0; + do { \ + if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL)) \ + newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length; \ + else if ((set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL) && \ + oldinfo->FIELD##_max_length && \ + !newinfo->FIELD##_max_length) \ + newinfo->FIELD##_max_length = (newinfo->FIELD || \ + newinfo->FIELD##_max) ? 1 : 0; \ + } while (0) SET_MAX_LENGTH(BYTES_MAX_LENGTH, total_bytes_sec); SET_MAX_LENGTH(BYTES_MAX_LENGTH, read_bytes_sec); -- 1.8.3.1

I'm going to get rid of macros code in qemuDomainSetBlockIoTune that converts virTypedParameter parameters into struct. In the scope of the overall effort to reduce/get rid of using macros when dealing with iotunes. And it will be much easier to use per field structure which hold whether field was set or not when removing macros in this place. So let's use just another virDomainBlockIoTuneInfo for this purpose where fields will hold bool values set/unset. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 100 +++++++++++++++++++++++++++---------------------- 1 file changed, 55 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 61be425..57d63b6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15886,26 +15886,32 @@ typedef enum { static int qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, virDomainBlockIoTuneInfoPtr oldinfo, - qemuBlockIoTuneSetFlags set_fields) + virDomainBlockIoTuneInfoPtr set_fields) { -#define SET_IOTUNE_DEFAULTS(BOOL, FIELD) \ +#define SET_IOTUNE_DEFAULTS(FIELD) \ do { \ - if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL)) { \ + bool set = set_fields->total_##FIELD || \ + set_fields->read_##FIELD || \ + set_fields->write_##FIELD; \ + if (!set) { \ newinfo->total_##FIELD = oldinfo->total_##FIELD; \ newinfo->read_##FIELD = oldinfo->read_##FIELD; \ newinfo->write_##FIELD = oldinfo->write_##FIELD; \ } \ } while (0) - SET_IOTUNE_DEFAULTS(BYTES, bytes_sec); - SET_IOTUNE_DEFAULTS(BYTES_MAX, bytes_sec_max); - SET_IOTUNE_DEFAULTS(IOPS, iops_sec); - SET_IOTUNE_DEFAULTS(IOPS_MAX, iops_sec_max); + SET_IOTUNE_DEFAULTS(bytes_sec); + SET_IOTUNE_DEFAULTS(bytes_sec_max); + SET_IOTUNE_DEFAULTS(iops_sec); + SET_IOTUNE_DEFAULTS(iops_sec_max); #undef SET_IOTUNE_DEFAULTS -#define RESET_IOTUNE_MAX(BOOL, FIELD) \ +#define RESET_IOTUNE_MAX(FIELD) \ do { \ - if (set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL) { \ + bool set = set_fields->total_##FIELD || \ + set_fields->read_##FIELD || \ + set_fields->write_##FIELD; \ + if (set) { \ if (!newinfo->total_##FIELD) \ newinfo->total_##FIELD##_max = 0; \ if (!newinfo->read_##FIELD) \ @@ -15915,11 +15921,11 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, } \ } while (0) - RESET_IOTUNE_MAX(BYTES, bytes_sec); - RESET_IOTUNE_MAX(IOPS, iops_sec); + RESET_IOTUNE_MAX(bytes_sec); + RESET_IOTUNE_MAX(iops_sec); #undef RESET_IOTUNE_MAX - if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS)) + if (!set_fields->size_iops_sec) newinfo->size_iops_sec = oldinfo->size_iops_sec; if (!newinfo->group_name) newinfo->group_name = g_strdup(oldinfo->group_name); @@ -15936,23 +15942,26 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, * will cause an error. So, to mimic that, if our oldinfo was set and * our newinfo is clearing, then set max_length based on whether we * have a value in the family set/defined. */ -#define SET_MAX_LENGTH(BOOL, FIELD) \ +#define SET_MAX_LENGTH(MAX_LENGTH_FIELD, FIELD) \ do { \ - if (!(set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL)) \ + bool set = set_fields->total_##MAX_LENGTH_FIELD || \ + set_fields->read_##MAX_LENGTH_FIELD || \ + set_fields->write_##MAX_LENGTH_FIELD; \ + if (!set) \ newinfo->FIELD##_max_length = oldinfo->FIELD##_max_length; \ - else if ((set_fields & QEMU_BLOCK_IOTUNE_SET_##BOOL) && \ + else if (set && \ oldinfo->FIELD##_max_length && \ !newinfo->FIELD##_max_length) \ newinfo->FIELD##_max_length = (newinfo->FIELD || \ newinfo->FIELD##_max) ? 1 : 0; \ } while (0) - SET_MAX_LENGTH(BYTES_MAX_LENGTH, total_bytes_sec); - SET_MAX_LENGTH(BYTES_MAX_LENGTH, read_bytes_sec); - SET_MAX_LENGTH(BYTES_MAX_LENGTH, write_bytes_sec); - SET_MAX_LENGTH(IOPS_MAX_LENGTH, total_iops_sec); - SET_MAX_LENGTH(IOPS_MAX_LENGTH, read_iops_sec); - SET_MAX_LENGTH(IOPS_MAX_LENGTH, write_iops_sec); + SET_MAX_LENGTH(bytes_sec_max_length, total_bytes_sec); + SET_MAX_LENGTH(bytes_sec_max_length, read_bytes_sec); + SET_MAX_LENGTH(bytes_sec_max_length, write_bytes_sec); + SET_MAX_LENGTH(iops_sec_max_length, total_iops_sec); + SET_MAX_LENGTH(iops_sec_max_length, read_iops_sec); + SET_MAX_LENGTH(iops_sec_max_length, write_iops_sec); #undef SET_MAX_LENGTH @@ -16044,7 +16053,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, size_t i; virDomainDiskDefPtr conf_disk = NULL; virDomainDiskDefPtr disk; - qemuBlockIoTuneSetFlags set_fields = 0; + virDomainBlockIoTuneInfo set_fields; bool supportMaxOptions = true; bool supportGroupNameOption = true; bool supportMaxLengthOptions = true; @@ -16105,6 +16114,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, memset(&info, 0, sizeof(info)); memset(&conf_info, 0, sizeof(conf_info)); + memset(&set_fields, 0, sizeof(set_fields)); if (!(vm = qemuDomainObjFromDomain(dom))) return -1; @@ -16126,10 +16136,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob; -#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ +#define SET_IOTUNE_FIELD(FIELD, CONST) \ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ info.FIELD = param->value.ul; \ - set_fields |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \ + set_fields.FIELD = 1; \ if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ &eventMaxparams, \ VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ @@ -16141,26 +16151,26 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, for (i = 0; i < nparams; i++) { virTypedParameterPtr param = &params[i]; - SET_IOTUNE_FIELD(total_bytes_sec, BYTES, TOTAL_BYTES_SEC); - SET_IOTUNE_FIELD(read_bytes_sec, BYTES, READ_BYTES_SEC); - SET_IOTUNE_FIELD(write_bytes_sec, BYTES, WRITE_BYTES_SEC); - SET_IOTUNE_FIELD(total_iops_sec, IOPS, TOTAL_IOPS_SEC); - SET_IOTUNE_FIELD(read_iops_sec, IOPS, READ_IOPS_SEC); - SET_IOTUNE_FIELD(write_iops_sec, IOPS, WRITE_IOPS_SEC); + SET_IOTUNE_FIELD(total_bytes_sec, TOTAL_BYTES_SEC); + SET_IOTUNE_FIELD(read_bytes_sec, READ_BYTES_SEC); + SET_IOTUNE_FIELD(write_bytes_sec, WRITE_BYTES_SEC); + SET_IOTUNE_FIELD(total_iops_sec, TOTAL_IOPS_SEC); + SET_IOTUNE_FIELD(read_iops_sec, READ_IOPS_SEC); + SET_IOTUNE_FIELD(write_iops_sec, WRITE_IOPS_SEC); - SET_IOTUNE_FIELD(total_bytes_sec_max, BYTES_MAX, + SET_IOTUNE_FIELD(total_bytes_sec_max, TOTAL_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(read_bytes_sec_max, BYTES_MAX, + SET_IOTUNE_FIELD(read_bytes_sec_max, READ_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(write_bytes_sec_max, BYTES_MAX, + SET_IOTUNE_FIELD(write_bytes_sec_max, WRITE_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(total_iops_sec_max, IOPS_MAX, + SET_IOTUNE_FIELD(total_iops_sec_max, TOTAL_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(read_iops_sec_max, IOPS_MAX, + SET_IOTUNE_FIELD(read_iops_sec_max, READ_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, + SET_IOTUNE_FIELD(write_iops_sec_max, WRITE_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC); + SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS_SEC); /* NB: Cannot use macro since this is a value.s not a value.ul */ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { @@ -16173,17 +16183,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, continue; } - SET_IOTUNE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH, + SET_IOTUNE_FIELD(total_bytes_sec_max_length, TOTAL_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH, + SET_IOTUNE_FIELD(read_bytes_sec_max_length, READ_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH, + SET_IOTUNE_FIELD(write_bytes_sec_max_length, WRITE_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH, + SET_IOTUNE_FIELD(total_iops_sec_max_length, TOTAL_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH, + SET_IOTUNE_FIELD(read_iops_sec_max_length, READ_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH, + SET_IOTUNE_FIELD(write_iops_sec_max_length, WRITE_IOPS_SEC_MAX_LENGTH); } @@ -16219,7 +16229,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info); if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, - set_fields) < 0) + &set_fields) < 0) goto endjob; if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0) @@ -16268,7 +16278,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, conf_cur_info = qemuDomainFindGroupBlockIoTune(persistentDef, conf_disk, &info); if (qemuDomainSetBlockIoTuneDefaults(&conf_info, conf_cur_info, - set_fields) < 0) + &set_fields) < 0) goto endjob; if (qemuDomainCheckBlockIoTuneReset(conf_disk, &conf_info) < 0) -- 1.8.3.1

At first glance we don't get much win because of introduction of virDomainBlockIoTuneFieldNames and virDomainBlockIoTuneFields. But we are going to use these two in other places to remove usage of macros too. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 99 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 30 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 800bca5..024d0e3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8695,40 +8695,80 @@ virDomainBlockIoTuneValidate(virDomainBlockIoTuneInfoPtr iotune) return 0; } -#define PARSE_IOTUNE(val) \ - if (virXPathULongLong("string(./iotune/" #val ")", \ - ctxt, &def->blkdeviotune.val) == -2) { \ - virReportError(VIR_ERR_XML_ERROR, \ - _("disk iotune field '%s' must be an integer"), #val); \ - return -1; \ - } + +static const char* virDomainBlockIoTuneFieldNames[] = { + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, +}; + + +static unsigned long long** +virDomainBlockIoTuneFields(virDomainBlockIoTuneInfoPtr iotune) +{ + unsigned long long **ret = g_new0(unsigned long long*, + G_N_ELEMENTS(virDomainBlockIoTuneFieldNames)); + size_t i = 0; + + ret[i++] = &iotune->total_bytes_sec; + ret[i++] = &iotune->read_bytes_sec; + ret[i++] = &iotune->write_bytes_sec; + ret[i++] = &iotune->total_iops_sec; + ret[i++] = &iotune->read_iops_sec; + ret[i++] = &iotune->write_iops_sec; + ret[i++] = &iotune->total_bytes_sec_max; + ret[i++] = &iotune->read_bytes_sec_max; + ret[i++] = &iotune->write_bytes_sec_max; + ret[i++] = &iotune->total_iops_sec_max; + ret[i++] = &iotune->read_iops_sec_max; + ret[i++] = &iotune->write_iops_sec_max; + ret[i++] = &iotune->size_iops_sec; + ret[i++] = &iotune->total_bytes_sec_max_length; + ret[i++] = &iotune->read_bytes_sec_max_length; + ret[i++] = &iotune->write_bytes_sec_max_length; + ret[i++] = &iotune->total_iops_sec_max_length; + ret[i++] = &iotune->read_iops_sec_max_length; + ret[i++] = &iotune->write_iops_sec_max_length; + + return ret; +} + static int virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, xmlXPathContextPtr ctxt) { - PARSE_IOTUNE(total_bytes_sec); - PARSE_IOTUNE(read_bytes_sec); - PARSE_IOTUNE(write_bytes_sec); - PARSE_IOTUNE(total_iops_sec); - PARSE_IOTUNE(read_iops_sec); - PARSE_IOTUNE(write_iops_sec); - - PARSE_IOTUNE(total_bytes_sec_max); - PARSE_IOTUNE(read_bytes_sec_max); - PARSE_IOTUNE(write_bytes_sec_max); - PARSE_IOTUNE(total_iops_sec_max); - PARSE_IOTUNE(read_iops_sec_max); - PARSE_IOTUNE(write_iops_sec_max); - - PARSE_IOTUNE(size_iops_sec); - - PARSE_IOTUNE(total_bytes_sec_max_length); - PARSE_IOTUNE(read_bytes_sec_max_length); - PARSE_IOTUNE(write_bytes_sec_max_length); - PARSE_IOTUNE(total_iops_sec_max_length); - PARSE_IOTUNE(read_iops_sec_max_length); - PARSE_IOTUNE(write_iops_sec_max_length); + g_autofree unsigned long long **fields = + virDomainBlockIoTuneFields(&def->blkdeviotune); + size_t i; + + for (i = 0; i < G_N_ELEMENTS(virDomainBlockIoTuneFieldNames); i++) { + const char *name = virDomainBlockIoTuneFieldNames[i]; + g_autofree char *sel = g_strdup_printf("string(./iotune/%s)", name); + + if (virXPathULongLong(sel, ctxt, fields[i]) == -2) { + virReportError(VIR_ERR_XML_ERROR, + _("disk iotune field '%s' must be an integer"), + name); + return -1; + } + } def->blkdeviotune.group_name = virXPathString("string(./iotune/group_name)", ctxt); @@ -8738,7 +8778,6 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, return 0; } -#undef PARSE_IOTUNE static int -- 1.8.3.1

ср, 3 мар. 2021 г. в 17:06, Peter Krempa <pkrempa(a)redhat.com&gt;: Hi, Peter. So it is finally a NACK as you don't say anything about this approach in later patches? Maybe there are other options to get rid of macros? At least I can factor out functions with macros to reuse them in qemu and test driver. I'll then make suggested changes for acked patches. Do I need to repost them or I can just push? Nikolay

It will be used as a replacement for macros code converting params to iotune structure. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 32 ++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bbe6ae7..aa6a2a8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31741,6 +31741,38 @@ virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, } +int +virDomainBlockIoTuneFromParams(virTypedParameterPtr params, + int nparams, + virDomainBlockIoTuneInfoPtr iotune, + virDomainBlockIoTuneInfoPtr set) +{ + g_autofree unsigned long long **fields = virDomainBlockIoTuneFields(iotune); + g_autofree unsigned long long **set_fields = virDomainBlockIoTuneFields(set); + const char *group_name = NULL; + size_t i; + + for (i = 0; i < G_N_ELEMENTS(virDomainBlockIoTuneFieldNames); i++) { + const char *name = virDomainBlockIoTuneFieldNames[i]; + int rc; + + rc = virTypedParamsGetULLong(params, nparams, name, fields[i]); + if (rc < 0) + return -1; + if (rc == 1) + *set_fields[i] = 1; + } + + if (virTypedParamsGetString(params, nparams, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + &group_name) < 0) + return -1; + iotune->group_name = g_strdup(group_name); + + return 0; +} + + /** * virHostdevIsSCSIDevice: * @hostdev: host device to check diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3c42313..e511939 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3949,6 +3949,12 @@ bool virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, const virDomainBlockIoTuneInfo *b); +int +virDomainBlockIoTuneFromParams(virTypedParameterPtr params, + int nparams, + virDomainBlockIoTuneInfoPtr iotune, + virDomainBlockIoTuneInfoPtr set); + bool virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, virDomainDiskBus bus_type, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bfe3ee7..bc42df0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -230,6 +230,7 @@ virDomainAudioTypeTypeFromString; virDomainAudioTypeTypeToString; virDomainBlockedReasonTypeFromString; virDomainBlockedReasonTypeToString; +virDomainBlockIoTuneFromParams; virDomainBlockIoTuneInfoCopy; virDomainBlockIoTuneInfoEqual; virDomainBlockIoTuneInfoHasAny; -- 1.8.3.1

Best viewed with --patience. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/qemu/qemu_driver.c | 72 ++++++-------------------------------------------- 1 file changed, 8 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 29c93de..8bcb876 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16039,7 +16039,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, g_autofree char *drivealias = NULL; const char *qdevid = NULL; int ret = -1; - size_t i; virDomainDiskDefPtr conf_disk = NULL; virDomainDiskDefPtr disk; virDomainBlockIoTuneInfo set_fields; @@ -16121,72 +16120,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; - if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) + if (virDomainBlockIoTuneFromParams(params, nparams, &info, &set_fields) < 0) goto endjob; -#define SET_IOTUNE_FIELD(FIELD, CONST) \ - if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ - info.FIELD = param->value.ul; \ - set_fields.FIELD = 1; \ - if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ - &eventMaxparams, \ - VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ - param->value.ul) < 0) \ - goto endjob; \ - continue; \ - } - - for (i = 0; i < nparams; i++) { - virTypedParameterPtr param = &params[i]; - - SET_IOTUNE_FIELD(total_bytes_sec, TOTAL_BYTES_SEC); - SET_IOTUNE_FIELD(read_bytes_sec, READ_BYTES_SEC); - SET_IOTUNE_FIELD(write_bytes_sec, WRITE_BYTES_SEC); - SET_IOTUNE_FIELD(total_iops_sec, TOTAL_IOPS_SEC); - SET_IOTUNE_FIELD(read_iops_sec, READ_IOPS_SEC); - SET_IOTUNE_FIELD(write_iops_sec, WRITE_IOPS_SEC); - - SET_IOTUNE_FIELD(total_bytes_sec_max, - TOTAL_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(read_bytes_sec_max, - READ_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(write_bytes_sec_max, - WRITE_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(total_iops_sec_max, - TOTAL_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(read_iops_sec_max, - READ_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(write_iops_sec_max, - WRITE_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS_SEC); - - /* NB: Cannot use macro since this is a value.s not a value.ul */ - if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - info.group_name = g_strdup(param->value.s); - if (virTypedParamsAddString(&eventParams, &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, - param->value.s) < 0) - goto endjob; - continue; - } - - SET_IOTUNE_FIELD(total_bytes_sec_max_length, - TOTAL_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_bytes_sec_max_length, - READ_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_bytes_sec_max_length, - WRITE_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(total_iops_sec_max_length, - TOTAL_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_iops_sec_max_length, - READ_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_iops_sec_max_length, - WRITE_IOPS_SEC_MAX_LENGTH); - } + if (virDomainBlockIoTuneToEventParams(&info, &set_fields, + &eventParams, + &eventNparams, &eventMaxparams) < 0) + goto endjob; -#undef SET_IOTUNE_FIELD + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) + goto endjob; if (virDomainBlockIoTuneValidate(&info) < 0) goto endjob; -- 1.8.3.1

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/test/test_driver.c | 97 +++++--------------------------------------------- 1 file changed, 9 insertions(+), 88 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index fba94b9..d0a7040 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3609,11 +3609,11 @@ testDomainSetBlockIoTune(virDomainPtr dom, virDomainObjPtr vm = NULL; virDomainDefPtr def = NULL; virDomainBlockIoTuneInfo info = {0}; + virDomainBlockIoTuneInfo set_fields = {0}; virDomainDiskDefPtr conf_disk = NULL; virTypedParameterPtr eventParams = NULL; int eventNparams = 0; int eventMaxparams = 0; - size_t i; int ret = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -3678,96 +3678,17 @@ testDomainSetBlockIoTune(virDomainPtr dom, info = conf_disk->blkdeviotune; - if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) + if (virDomainBlockIoTuneFromParams(params, nparams, &info, &set_fields) < 0) goto cleanup; -#define SET_IOTUNE_FIELD(FIELD, STR, TUNABLE_STR) \ - if (STREQ(param->field, STR)) { \ - info.FIELD = param->value.ul; \ - if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ - &eventMaxparams, \ - TUNABLE_STR, \ - param->value.ul) < 0) \ - goto cleanup; \ - continue; \ - } - - for (i = 0; i < nparams; i++) { - virTypedParameterPtr param = &params[i]; - - SET_IOTUNE_FIELD(total_bytes_sec, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC); - SET_IOTUNE_FIELD(read_bytes_sec, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC); - SET_IOTUNE_FIELD(write_bytes_sec, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC); - SET_IOTUNE_FIELD(total_iops_sec, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC); - SET_IOTUNE_FIELD(read_iops_sec, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC); - SET_IOTUNE_FIELD(write_iops_sec, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC); - - SET_IOTUNE_FIELD(total_bytes_sec_max, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(read_bytes_sec_max, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(write_bytes_sec_max, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX); - SET_IOTUNE_FIELD(total_iops_sec_max, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(read_iops_sec_max, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(write_iops_sec_max, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX); - SET_IOTUNE_FIELD(size_iops_sec, - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC); - - if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - info.group_name = g_strdup(param->value.s); - if (virTypedParamsAddString(&eventParams, - &eventNparams, - &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, - param->value.s) < 0) - goto cleanup; - continue; - } + if (virDomainBlockIoTuneToEventParams(&info, &set_fields, + &eventParams, + &eventNparams, &eventMaxparams) < 0) + goto cleanup; - SET_IOTUNE_FIELD(total_bytes_sec_max_length, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_bytes_sec_max_length, - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_bytes_sec_max_length, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(total_iops_sec_max_length, - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(read_iops_sec_max_length, - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH); - SET_IOTUNE_FIELD(write_iops_sec_max_length, - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH); - } -#undef SET_IOTUNE_FIELD + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) + goto cleanup; if (!info.group_name) info.group_name = g_strdup(conf_disk->blkdeviotune.group_name); -- 1.8.3.1