[PATCH 00/23] RFC: get rid of macros when dealing with block io tunes
Hi, all. I started this work as adding missing parts/fixing issues/etc in block iotune code but then turned to refactoring code. We use a lot of macros in this place and if we get rid of them I belive we will have much more readable/reusable/ extendable code. Most of macros usage is for iterating over unsigned long long values. I'm talking about parsing/formating xml, converting from/to virTypedParameterPtr list etc. These places do not care about tune semantics and thus we can add tools for the said iteration. See patch [1] for the first such conversion. Patches before it partially prepare for this conversion, partially just improve code reuse and add missing parts. The work on removing macros in code handling iotunes is not finished as I wanted to get an approvement that I have taken a right direction. At the same time series shows the potential of the approach (take a look on how qemuDomainSetBlockIoTune and testDomainSetBlockIoTune are looking now!). Some places like qemuDomainSetBlockIoTuneDefaults deal with fields semantics. So we need another approach to remove macros there but it is a matter of a different RFC. Nikolay Shirokovskiy (23): qemu: pass supportGroupNameOption as expected qemu: factor out qemuValidateDomainBlkdeviotune qemu: reuse validation in qemuDomainSetBlockIoTune qemu: remove extra check for QEMU_BLOCK_IOTUNE_MAX conf: factor out virDomainBlockIoTuneValidate qemu: reuse virDomainBlockIoTuneValidate test driver: reuse virDomainBlockIoTuneValidate qemu: reset max iotune setting when needed qemu: add max iotune settings check to virDomainBlockIoTuneValidate qemu: remove iotune max checks test driver: remove iotune max checks qemu: prepare for removing qemuBlockIoTuneSetFlags qemu: use group_name instead of QEMU_BLOCK_IOTUNE_SET_GROUP_NAME qemu: remove qemuBlockIoTuneSetFlags usage in qemuDomainSetBlockIoTune qemu: remove qemuBlockIoTuneSetFlags enum completly conf: get rid of macros in virDomainDiskDefIotuneParse [1] conf: get rid of macros in virDomainDiskDefFormatIotune conf: add virDomainBlockIoTuneFromParams conf: add virDomainBlockIoTuneToEventParams qemu: qemuDomainSetBlockIoTune use functions to convert to/from params test driver: remove TEST_BLOCK_IOTUNE_MAX checks test driver: prepare to delete macros in testDomainSetBlockIoTune test driver: testDomainSetBlockIoTune: use functions to convert to/from params src/conf/domain_conf.c | 303 +++++++++++++++++++++++++++++++++-------------- src/conf/domain_conf.h | 16 +++ src/libvirt_private.syms | 3 + src/qemu/qemu_driver.c | 281 ++++++++++++------------------------------- src/qemu/qemu_validate.c | 100 +++++++++------- src/qemu/qemu_validate.h | 4 + src/test/test_driver.c | 156 ++---------------------- 7 files changed, 380 insertions(+), 483 deletions(-) -- 1.8.3.1
supportGroupNameOption was originally passed before the below patch. To solve the issue of that patch this change is not necessary and it make sense to pass supportGroupNameOption because of qemuMonitorSetBlockIoThrottle signature. With patch group_name is passed to qemu if disk in iotune group but group is not passed explicitly in parametes. For qemu it does not make a difference. I'm preparing here for the next patch where another usage of supportGroupNameOption will gone and instead of deleting supportGroupNameOption variable altogether I guess it is better to restore it's proper usage. commit e9d75343d4cf552575a3b305fa00a36ee71e9309 Author: Martin Kletzander <mkletzan@redhat.com> Date: Tue Jan 24 15:50:00 2017 +0100 qemu: Only set group_name when actually requested Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 027617d..b7f22c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16310,7 +16310,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid, &info, supportMaxOptions, - set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME, + supportGroupNameOption, supportMaxLengthOptions); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; -- 1.8.3.1
On Mon, Jan 11, 2021 at 12:49:54 +0300, Nikolay Shirokovskiy wrote:
supportGroupNameOption was originally passed before the below patch. To solve the issue of that patch this change is not necessary and it make sense to pass supportGroupNameOption because of qemuMonitorSetBlockIoThrottle signature.
With patch group_name is passed to qemu if disk in iotune group but group is not passed explicitly in parametes. For qemu it does not make a difference.
We always set all the fields in qemu, so arguably this is fixing the behaviour also for the group name. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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>
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@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:49:56 +0300, Nikolay Shirokovskiy wrote:
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.
I'd say that it's an acceptable change.
Second, check for QEMU_BLOCK_IOTUNE_MAX is added but it is alredy checked above and I'm going to remove the above check.
I'd not mention this.
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@virtuozzo.com> --- src/qemu/qemu_driver.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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@virtuozzo.com> --- 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 = ¶ms[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
On Mon, Jan 11, 2021 at 12:49:57 +0300, Nikolay Shirokovskiy wrote:
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.
fair enough.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 7 ------- 1 file changed, 7 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
virDomainBlockIoTuneValidate can be reused in virDomainSetBlockIoTune implementations. And also simplify if conditions. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:49:58 +0300, Nikolay Shirokovskiy wrote:
virDomainBlockIoTuneValidate can be reused in virDomainSetBlockIoTune implementations.
And also simplify if conditions.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
[...]
+ #define PARSE_IOTUNE(val) \ if (virXPathULongLong("string(./iotune/" #val ")", \ ctxt, &def->blkdeviotune.val) == -2) { \ @@ -8665,45 +8704,8 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
[...]
- 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;
This should be called from 'virDomainDiskDefValidate' rather than from the parser. With that: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:49:59 +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:50:00 +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/test/test_driver.c | 32 +------------------------------- 1 file changed, 1 insertion(+), 31 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:50:01 +0300, Nikolay Shirokovskiy wrote:
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.
NACK, IMO we must not change any fields which the user didn't touch.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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) \
Also the series sells removing macros, so adding new ones isn't very productive ;)
+ 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
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@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:50:02 +0300, Nikolay Shirokovskiy wrote:
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@virtuozzo.com> --- 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) \
As noted for previous patch, this series is meant to remove macros, not add them. Add an open-coded version.
+ do { \ + if (iotune->val##_max) { \ + if (!iotune->val) { \ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ + _("value '%s' cannot be set if " \ + "'%s' is not set"), \
And don't break error messages.
These checks are now in the above call to virDomainBlockIoTuneValidate. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:50:03 +0300, Nikolay Shirokovskiy wrote:
These checks are now in the above call to virDomainBlockIoTuneValidate.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 29 ----------------------------- 1 file changed, 29 deletions(-)
Once there is a replacement for previous patch whithout macros: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
These checks are now in the above call to virDomainBlockIoTuneValidate. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:50:04 +0300, Nikolay Shirokovskiy wrote:
These checks are now in the above call to virDomainBlockIoTuneValidate.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/test/test_driver.c | 19 ------------------- 1 file changed, 19 deletions(-)
Under same conditions as on 10/23: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We need extra variable in macros thus let's add do {} while (0) wrapping. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
We need qemuBlockIoTuneSetFlags to distinguish cases when some tune is not set from tune is set explicitly to 0. We don't have the latter case for the group name and don't need QEMU_BLOCK_IOTUNE_SET_GROUP_NAME. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 15ccf90..61be425 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15921,7 +15921,7 @@ qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, 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)) + if (!newinfo->group_name) newinfo->group_name = g_strdup(oldinfo->group_name); /* The length field is handled a bit differently. If not defined/set, @@ -16165,7 +16165,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, /* 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); - set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, -- 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@virtuozzo.com> --- 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 = ¶ms[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
It is not used anymore. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57d63b6..29c93de 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15869,17 +15869,6 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, return ret; } -typedef enum { - QEMU_BLOCK_IOTUNE_SET_BYTES = 1 << 0, - QEMU_BLOCK_IOTUNE_SET_IOPS = 1 << 1, - QEMU_BLOCK_IOTUNE_SET_BYTES_MAX = 1 << 2, - QEMU_BLOCK_IOTUNE_SET_IOPS_MAX = 1 << 3, - QEMU_BLOCK_IOTUNE_SET_SIZE_IOPS = 1 << 4, - QEMU_BLOCK_IOTUNE_SET_GROUP_NAME = 1 << 5, - QEMU_BLOCK_IOTUNE_SET_BYTES_MAX_LENGTH = 1 << 6, - QEMU_BLOCK_IOTUNE_SET_IOPS_MAX_LENGTH = 1 << 7, -} qemuBlockIoTuneSetFlags; - /* If the user didn't specify bytes limits, inherit previous values; * likewise if the user didn't specify iops limits. */ -- 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@virtuozzo.com> --- 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
On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote:
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@virtuozzo.com> --- 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);
IMO this is worse than we had before. I'm especially not a fan of correlating arrays into named fields and the parser is actually harder to understand. Let's see the other places you are describing, but I don't think you can offset the damage done by correlating two arrays.
ср, 3 мар. 2021 г. в 17:06, Peter Krempa <pkrempa@redhat.com>:
On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote:
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@virtuozzo.com> --- 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);
IMO this is worse than we had before. I'm especially not a fan of correlating arrays into named fields and the parser is actually harder to understand.
Let's see the other places you are describing, but I don't think you can offset the damage done by correlating two arrays.
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
On Tue, Mar 09, 2021 at 10:38:36 +0300, Nikolay Shirokovskiy wrote:
ср, 3 мар. 2021 г. в 17:06, Peter Krempa <pkrempa@redhat.com>:
On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote:
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@virtuozzo.com> --- 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);
IMO this is worse than we had before. I'm especially not a fan of correlating arrays into named fields and the parser is actually harder to understand.
Let's see the other places you are describing, but I don't think you can offset the damage done by correlating two arrays.
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?
It's a NACK for +virDomainBlockIoTuneFields and it's use in the XML parser and formatter which makes it way worse than it was before. IMO a better solution is to have two functions which will convert the virTypedParams to a blkdeviotune structure and back (if needed), and use them instead of the opencoded stuff. Additionally I don't like the storage of whether a field was specified in another copy of the blkdeviotune struct filled with 1 and 0 values, because it's again obscure. A possibility is to create an extended structure which will include blkdeviotune and then booleans specifying whether it's presnt or not, or extend the original struct.
On Tue, Mar 09, 2021 at 10:38:36 +0300, Nikolay Shirokovskiy wrote:
ср, 3 мар. 2021 г. в 17:06, Peter Krempa <pkrempa@redhat.com>:
On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote:
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@virtuozzo.com> --- src/conf/domain_conf.c | 99 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 30 deletions(-)
[...] Oops deleted too much previously.
I'll then make suggested changes for acked patches. Do I need to repost them or I can just push?
You can push them right away.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 42 +++++++++--------------------------------- 1 file changed, 9 insertions(+), 33 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 024d0e3..bbe6ae7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24225,54 +24225,30 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, } -#define FORMAT_IOTUNE(val) \ - if (disk->blkdeviotune.val) { \ - virBufferAsprintf(&childBuf, "<" #val ">%llu</" #val ">\n", \ - disk->blkdeviotune.val); \ - } - static void virDomainDiskDefFormatIotune(virBufferPtr buf, virDomainDiskDefPtr disk) { g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + g_autofree unsigned long long **fields = + virDomainBlockIoTuneFields(&disk->blkdeviotune); + size_t i; - FORMAT_IOTUNE(total_bytes_sec); - FORMAT_IOTUNE(read_bytes_sec); - FORMAT_IOTUNE(write_bytes_sec); - FORMAT_IOTUNE(total_iops_sec); - FORMAT_IOTUNE(read_iops_sec); - FORMAT_IOTUNE(write_iops_sec); - - FORMAT_IOTUNE(total_bytes_sec_max); - FORMAT_IOTUNE(read_bytes_sec_max); - FORMAT_IOTUNE(write_bytes_sec_max); - FORMAT_IOTUNE(total_iops_sec_max); - FORMAT_IOTUNE(read_iops_sec_max); - FORMAT_IOTUNE(write_iops_sec_max); + for (i = 0; i < G_N_ELEMENTS(virDomainBlockIoTuneFieldNames); i++) { + const char *name = virDomainBlockIoTuneFieldNames[i]; - if (disk->blkdeviotune.size_iops_sec) { - virBufferAsprintf(&childBuf, "<size_iops_sec>%llu</size_iops_sec>\n", - disk->blkdeviotune.size_iops_sec); + if (*fields[i]) + virBufferAsprintf(&childBuf, "<%s>%llu</%s>\n", + name, *fields[i], name); } - if (disk->blkdeviotune.group_name) { + if (disk->blkdeviotune.group_name) virBufferEscapeString(&childBuf, "<group_name>%s</group_name>\n", disk->blkdeviotune.group_name); - } - - FORMAT_IOTUNE(total_bytes_sec_max_length); - FORMAT_IOTUNE(read_bytes_sec_max_length); - FORMAT_IOTUNE(write_bytes_sec_max_length); - FORMAT_IOTUNE(total_iops_sec_max_length); - FORMAT_IOTUNE(read_iops_sec_max_length); - FORMAT_IOTUNE(write_iops_sec_max_length); virXMLFormatElement(buf, "iotune", NULL, &childBuf); } -#undef FORMAT_IOTUNE - static void virDomainDiskDefFormatDriver(virBufferPtr buf, -- 1.8.3.1
It will be used as a replacement for macros code converting params to iotune structure. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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
It will be used as a replacement for macros code adding event params. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 7 +++++++ src/libvirt_private.syms | 1 + 3 files changed, 61 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa6a2a8..7e91654 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31773,6 +31773,59 @@ virDomainBlockIoTuneFromParams(virTypedParameterPtr params, } +static const char* virDomainBlockIoTuneFieldEventNames[] = { + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX, + VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH, + VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH, + VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH, + VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH, +}; + + +int +virDomainBlockIoTuneToEventParams(virDomainBlockIoTuneInfoPtr iotune, + virDomainBlockIoTuneInfoPtr set, + virTypedParameterPtr *params, + int *nparams, + int *maxparams) +{ + g_autofree unsigned long long **fields = virDomainBlockIoTuneFields(iotune); + g_autofree unsigned long long **set_fields = virDomainBlockIoTuneFields(set); + size_t i; + + for (i = 0; i < G_N_ELEMENTS(virDomainBlockIoTuneFieldEventNames); i++) { + const char *name = virDomainBlockIoTuneFieldEventNames[i]; + + if (*set_fields[i] && + virTypedParamsAddULLong(params, nparams, maxparams, + name, *fields[i]) < 0) + return -1; + } + + if (iotune->group_name && + virTypedParamsAddString(params, nparams, maxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + iotune->group_name) < 0) + return -1; + + return 0; +} + + /** * virHostdevIsSCSIDevice: * @hostdev: host device to check diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e511939..ea18720 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3955,6 +3955,13 @@ virDomainBlockIoTuneFromParams(virTypedParameterPtr params, virDomainBlockIoTuneInfoPtr iotune, virDomainBlockIoTuneInfoPtr set); +int +virDomainBlockIoTuneToEventParams(virDomainBlockIoTuneInfoPtr iotune, + virDomainBlockIoTuneInfoPtr set, + virTypedParameterPtr *params, + int *nparams, + int *maxparams); + bool virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, virDomainDiskBus bus_type, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bc42df0..e71b580 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -237,6 +237,7 @@ virDomainBlockIoTuneInfoHasAny; virDomainBlockIoTuneInfoHasBasic; virDomainBlockIoTuneInfoHasMax; virDomainBlockIoTuneInfoHasMaxLength; +virDomainBlockIoTuneToEventParams; virDomainBlockIoTuneValidate; virDomainBootTypeFromString; virDomainBootTypeToString; -- 1.8.3.1
Best viewed with --patience. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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 = ¶ms[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
The check is copied from qemu driver I guess and does not make much sense for test driver. This patch is a preparation step to get rid of macros in this place. And I guess it make sence just to drop this check instead of moving to some function. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/test/test_driver.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 299be2a..58d3d79 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3599,8 +3599,6 @@ testDomainGetInterfaceParameters(virDomainPtr dom, } -#define TEST_BLOCK_IOTUNE_MAX 1000000000000000LL - static int testDomainSetBlockIoTune(virDomainPtr dom, const char *path, @@ -3699,13 +3697,6 @@ testDomainSetBlockIoTune(virDomainPtr dom, for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; - if (param->value.ul > TEST_BLOCK_IOTUNE_MAX) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, - _("block I/O throttle limit value must" - " be no more than %llu"), TEST_BLOCK_IOTUNE_MAX); - goto cleanup; - } - SET_IOTUNE_FIELD(total_bytes_sec, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC); -- 1.8.3.1
On Mon, Jan 11, 2021 at 12:50:14 +0300, Nikolay Shirokovskiy wrote:
The check is copied from qemu driver I guess and does not make much sense for test driver. This patch is a preparation step to get rid of macros in this place. And I guess it make sence just to drop this check instead of moving to
s/sence/sense/
some function.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/test/test_driver.c | 9 --------- 1 file changed, 9 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We are going to delete macros for converting from params/adding event params. Thus let's make macros conform to existing virDomainBlockIoTuneFromParams. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/test/test_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 58d3d79..fba94b9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3677,7 +3677,6 @@ testDomainSetBlockIoTune(virDomainPtr dom, } info = conf_disk->blkdeviotune; - info.group_name = g_strdup(conf_disk->blkdeviotune.group_name); if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) @@ -3739,7 +3738,6 @@ testDomainSetBlockIoTune(virDomainPtr dom, VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC); if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - VIR_FREE(info.group_name); info.group_name = g_strdup(param->value.s); if (virTypedParamsAddString(&eventParams, &eventNparams, @@ -3771,6 +3769,9 @@ testDomainSetBlockIoTune(virDomainPtr dom, } #undef SET_IOTUNE_FIELD + if (!info.group_name) + info.group_name = g_strdup(conf_disk->blkdeviotune.group_name); + if (virDomainBlockIoTuneValidate(&info) < 0) goto cleanup; -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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 = ¶ms[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
Polite ping. On Mon, Jan 11, 2021 at 12:49 PM Nikolay Shirokovskiy < nshirokovskiy@virtuozzo.com> wrote:
Hi, all.
I started this work as adding missing parts/fixing issues/etc in block iotune code but then turned to refactoring code. We use a lot of macros in this place and if we get rid of them I belive we will have much more readable/reusable/ extendable code.
Most of macros usage is for iterating over unsigned long long values. I'm talking about parsing/formating xml, converting from/to virTypedParameterPtr list etc. These places do not care about tune semantics and thus we can add tools for the said iteration. See patch [1] for the first such conversion.
Patches before it partially prepare for this conversion, partially just improve code reuse and add missing parts.
The work on removing macros in code handling iotunes is not finished as I wanted to get an approvement that I have taken a right direction. At the same time series shows the potential of the approach (take a look on how qemuDomainSetBlockIoTune and testDomainSetBlockIoTune are looking now!).
Some places like qemuDomainSetBlockIoTuneDefaults deal with fields semantics. So we need another approach to remove macros there but it is a matter of a different RFC.
Nikolay Shirokovskiy (23): qemu: pass supportGroupNameOption as expected qemu: factor out qemuValidateDomainBlkdeviotune qemu: reuse validation in qemuDomainSetBlockIoTune qemu: remove extra check for QEMU_BLOCK_IOTUNE_MAX conf: factor out virDomainBlockIoTuneValidate qemu: reuse virDomainBlockIoTuneValidate test driver: reuse virDomainBlockIoTuneValidate qemu: reset max iotune setting when needed qemu: add max iotune settings check to virDomainBlockIoTuneValidate qemu: remove iotune max checks test driver: remove iotune max checks qemu: prepare for removing qemuBlockIoTuneSetFlags qemu: use group_name instead of QEMU_BLOCK_IOTUNE_SET_GROUP_NAME qemu: remove qemuBlockIoTuneSetFlags usage in qemuDomainSetBlockIoTune qemu: remove qemuBlockIoTuneSetFlags enum completly conf: get rid of macros in virDomainDiskDefIotuneParse [1] conf: get rid of macros in virDomainDiskDefFormatIotune conf: add virDomainBlockIoTuneFromParams conf: add virDomainBlockIoTuneToEventParams qemu: qemuDomainSetBlockIoTune use functions to convert to/from params test driver: remove TEST_BLOCK_IOTUNE_MAX checks test driver: prepare to delete macros in testDomainSetBlockIoTune test driver: testDomainSetBlockIoTune: use functions to convert to/from params
src/conf/domain_conf.c | 303 +++++++++++++++++++++++++++++++++-------------- src/conf/domain_conf.h | 16 +++ src/libvirt_private.syms | 3 + src/qemu/qemu_driver.c | 281 ++++++++++++------------------------------- src/qemu/qemu_validate.c | 100 +++++++++------- src/qemu/qemu_validate.h | 4 + src/test/test_driver.c | 156 ++---------------------- 7 files changed, 380 insertions(+), 483 deletions(-)
-- 1.8.3.1
A ping to save this series from falling thru the cracks. ср, 27 янв. 2021 г. в 11:19, Nikolay Shirokovskiy < nshirokovskiy@virtuozzo.com>:
Polite ping.
On Mon, Jan 11, 2021 at 12:49 PM Nikolay Shirokovskiy < nshirokovskiy@virtuozzo.com> wrote:
Hi, all.
I started this work as adding missing parts/fixing issues/etc in block iotune code but then turned to refactoring code. We use a lot of macros in this place and if we get rid of them I belive we will have much more readable/reusable/ extendable code.
Most of macros usage is for iterating over unsigned long long values. I'm talking about parsing/formating xml, converting from/to virTypedParameterPtr list etc. These places do not care about tune semantics and thus we can add tools for the said iteration. See patch [1] for the first such conversion.
Patches before it partially prepare for this conversion, partially just improve code reuse and add missing parts.
The work on removing macros in code handling iotunes is not finished as I wanted to get an approvement that I have taken a right direction. At the same time series shows the potential of the approach (take a look on how qemuDomainSetBlockIoTune and testDomainSetBlockIoTune are looking now!).
Some places like qemuDomainSetBlockIoTuneDefaults deal with fields semantics. So we need another approach to remove macros there but it is a matter of a different RFC.
Nikolay Shirokovskiy (23): qemu: pass supportGroupNameOption as expected qemu: factor out qemuValidateDomainBlkdeviotune qemu: reuse validation in qemuDomainSetBlockIoTune qemu: remove extra check for QEMU_BLOCK_IOTUNE_MAX conf: factor out virDomainBlockIoTuneValidate qemu: reuse virDomainBlockIoTuneValidate test driver: reuse virDomainBlockIoTuneValidate qemu: reset max iotune setting when needed qemu: add max iotune settings check to virDomainBlockIoTuneValidate qemu: remove iotune max checks test driver: remove iotune max checks qemu: prepare for removing qemuBlockIoTuneSetFlags qemu: use group_name instead of QEMU_BLOCK_IOTUNE_SET_GROUP_NAME qemu: remove qemuBlockIoTuneSetFlags usage in qemuDomainSetBlockIoTune qemu: remove qemuBlockIoTuneSetFlags enum completly conf: get rid of macros in virDomainDiskDefIotuneParse [1] conf: get rid of macros in virDomainDiskDefFormatIotune conf: add virDomainBlockIoTuneFromParams conf: add virDomainBlockIoTuneToEventParams qemu: qemuDomainSetBlockIoTune use functions to convert to/from params test driver: remove TEST_BLOCK_IOTUNE_MAX checks test driver: prepare to delete macros in testDomainSetBlockIoTune test driver: testDomainSetBlockIoTune: use functions to convert to/from params
src/conf/domain_conf.c | 303 +++++++++++++++++++++++++++++++++-------------- src/conf/domain_conf.h | 16 +++ src/libvirt_private.syms | 3 + src/qemu/qemu_driver.c | 281 ++++++++++++------------------------------- src/qemu/qemu_validate.c | 100 +++++++++------- src/qemu/qemu_validate.h | 4 + src/test/test_driver.c | 156 ++---------------------- 7 files changed, 380 insertions(+), 483 deletions(-)
-- 1.8.3.1
participants (2)
-
Nikolay Shirokovskiy -
Peter Krempa