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