ср, 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