Re: [PATCH 16/23] conf: get rid of macros in virDomainDiskDefIotuneParse

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(a)virtuozzo.com&gt; > --- > src/conf/domain_conf.c | 99 +++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 69 insertions(+), 30 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 800bca5..024d0e3 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -8695,40 +8695,80 @@ virDomainBlockIoTuneValidate(virDomainBlockIoTuneInfoPtr iotune) > return 0; > } > > -#define PARSE_IOTUNE(val) \ > - if (virXPathULongLong("string(./iotune/" #val ")", \ > - ctxt, &def->blkdeviotune.val) == -2) { \ > - virReportError(VIR_ERR_XML_ERROR, \ > - _("disk iotune field '%s' must be an integer"), #val); \ > - return -1; \ > - } > + > +static const char* virDomainBlockIoTuneFieldNames[] = { > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, > + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, > +}; > + > + > +static unsigned long long** > +virDomainBlockIoTuneFields(virDomainBlockIoTuneInfoPtr iotune) > +{ > + unsigned long long **ret = g_new0(unsigned long long*, > + G_N_ELEMENTS(virDomainBlockIoTuneFieldNames)); > + size_t i = 0; > + > + ret[i++] = &iotune->total_bytes_sec; > + ret[i++] = &iotune->read_bytes_sec; > + ret[i++] = &iotune->write_bytes_sec; > + ret[i++] = &iotune->total_iops_sec; > + ret[i++] = &iotune->read_iops_sec; > + ret[i++] = &iotune->write_iops_sec; > + ret[i++] = &iotune->total_bytes_sec_max; > + ret[i++] = &iotune->read_bytes_sec_max; > + ret[i++] = &iotune->write_bytes_sec_max; > + ret[i++] = &iotune->total_iops_sec_max; > + ret[i++] = &iotune->read_iops_sec_max; > + ret[i++] = &iotune->write_iops_sec_max; > + ret[i++] = &iotune->size_iops_sec; > + ret[i++] = &iotune->total_bytes_sec_max_length; > + ret[i++] = &iotune->read_bytes_sec_max_length; > + ret[i++] = &iotune->write_bytes_sec_max_length; > + ret[i++] = &iotune->total_iops_sec_max_length; > + ret[i++] = &iotune->read_iops_sec_max_length; > + ret[i++] = &iotune->write_iops_sec_max_length; > + > + return ret; > +} > + > > static int > virDomainDiskDefIotuneParse(virDomainDiskDefPtr def, > xmlXPathContextPtr ctxt) > { > - PARSE_IOTUNE(total_bytes_sec); > - PARSE_IOTUNE(read_bytes_sec); > - PARSE_IOTUNE(write_bytes_sec); > - PARSE_IOTUNE(total_iops_sec); > - PARSE_IOTUNE(read_iops_sec); > - PARSE_IOTUNE(write_iops_sec); > - > - PARSE_IOTUNE(total_bytes_sec_max); > - PARSE_IOTUNE(read_bytes_sec_max); > - PARSE_IOTUNE(write_bytes_sec_max); > - PARSE_IOTUNE(total_iops_sec_max); > - PARSE_IOTUNE(read_iops_sec_max); > - PARSE_IOTUNE(write_iops_sec_max); > - > - PARSE_IOTUNE(size_iops_sec); > - > - PARSE_IOTUNE(total_bytes_sec_max_length); > - PARSE_IOTUNE(read_bytes_sec_max_length); > - PARSE_IOTUNE(write_bytes_sec_max_length); > - PARSE_IOTUNE(total_iops_sec_max_length); > - PARSE_IOTUNE(read_iops_sec_max_length); > - PARSE_IOTUNE(write_iops_sec_max_length); > + g_autofree unsigned long long **fields = > + virDomainBlockIoTuneFields(&def->blkdeviotune); > + size_t i; > + > + for (i = 0; i < G_N_ELEMENTS(virDomainBlockIoTuneFieldNames); i++) { > + const char *name = virDomainBlockIoTuneFieldNames[i]; > + g_autofree char *sel = g_strdup_printf("string(./iotune/%s)", name); > + > + if (virXPathULongLong(sel, ctxt, fields[i]) == -2) { > + virReportError(VIR_ERR_XML_ERROR, > + _("disk iotune field '%s' must be an integer"), > + name); > + return -1; > + } > + } > > def->blkdeviotune.group_name = > virXPathString("string(./iotune/group_name)", ctxt); 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.