On Tue, Mar 09, 2021 at 10:38:36 +0300, Nikolay Shirokovskiy wrote:
> ср, 3 мар. 2021 г. в 17:06, Peter Krempa <pkrempa@xxxxxxxxxx>:
>
> > 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@xxxxxxxxxxxxx>
> > > ---
> > > 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.
