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

ср, 3 мар. 2021 г. в 17:06, Peter Krempa <pkrempa(a)redhat.com&gt;: > 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. > > 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?