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.