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.