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

[Nikolay Shirokovskiy]

ср, 3 мар. 2021 г. в 17:06, Peter Krempa <pkrempa at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210309/341647c0/attachment-0001.htm>