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);
@@ -8738,7 +8778,6 @@ virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,
 
     return 0;
 }
-#undef PARSE_IOTUNE
 
 
 static int
-- 
1.8.3.1

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.

ср, 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?

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

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.

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(-)

[...]

Oops deleted too much previously.

> I'll then make suggested changes for acked patches. Do I need to repost
> them or I can just push?

You can push them right away.